Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRL returns empty for vault versions >= 1.11.0 #20137

Closed
flassman opened this issue Apr 13, 2023 · 15 comments · Fixed by #20253
Closed

CRL returns empty for vault versions >= 1.11.0 #20137

flassman opened this issue Apr 13, 2023 · 15 comments · Fixed by #20253

Comments

@flassman
Copy link

Describe the bug
After upgrading from 1.5.4 to 1.13.1 we found that querying any CRL with curl (like from https://vault.example.com/v1/my-pki/crl/pem) returns empty, status code 204.
Trying different vault versions it turned out that it works up to 1.10.11 and starts to fail with 1.11.0
In the pod log there is this warning for each such query:

2023-04-13T10:14:05.514Z [WARN]  secrets.pki.pki_33b4283f: possible error, but cannot return in raw response. Note that an empty CA probably means none was configured, and an empty CRL is possibly correct: error="unable to find CRL for issuer: id:07864a16-a812-9879-6380-7679aa0d0719/ref:default"

Not sure if it is related: an API LIST on my-pki/certs/revoked also returns nothing for 1.11.x and 1.12.x but it works for 1.13.1 (where the CRL query still returns 204)

To Reproduce
I fear this can only reproduced with my vault data/config?!

Expected behavior
Obviously I want to get the CRL containing the revoked certificates (or an empty CRL) just as with vault <= 1.10.11

Environment:

  • Vault Server Version (retrieve with vault status): 1.11.0
  • Vault CLI Version (retrieve with vault version): 1.11.0
  • Server Operating System/Architecture: k8s, image: hashicorp/vault:1.10
    .0

Vault server configuration file(s):

disable_mlock = true
ui = true

listener "tcp" {
  tls_disable = 1
  address = "[::]:8200"
  cluster_address = "[::]:8201"
}
storage "file" {
  path = "/vault/data"
}

Additional context
Happy to help debug this, let me know if you need more information.

@cipherboy
Copy link
Contributor

cipherboy commented Apr 14, 2023

@flassman Can you share your list of issuers, your CRL config, and your list of keys? It'd be good to know if the issuers include key_id values in their entries as well vault read pki/issuer/default. What happens if you attempt to manually rebuild the CRL?


Re: API LIST on my-pki/certs/revoked, this endpoint was only added on 1.13 I believe, so this is not surprising.

@flassman
Copy link
Author

flassman commented Apr 17, 2023

list of issuers
http://127.0.0.1:8200/v1/my-pki/issuers?list=true

{
  "request_id": "7739a2b4-f314-b03d-9b78-90029bd61c7b",
  "lease_id": "",
  "renewable": false,
  "lease_duration": 0,
  "data": {
    "key_info": {
      "09a41e7f-9e25-9f99-6601-7e612636f2f7": {
        "is_default": true,
        "issuer_name": "current-1681402889"
      }
    },
    "keys": [
      "09a41e7f-9e25-9f99-6601-7e612636f2f7"
    ]
  },
  "wrap_info": null,
  "warnings": null,
  "auth": null
}

CRL config
http://127.0.0.1:8200/v1/my-pki/config/crl

{
  "request_id": "625255d0-59c9-8164-78c8-a2869629e222",
  "lease_id": "",
  "renewable": false,
  "lease_duration": 0,
  "data": {
    "auto_rebuild": false,
    "auto_rebuild_grace_period": "12h",
    "cross_cluster_revocation": false,
    "delta_rebuild_interval": "15m",
    "disable": false,
    "enable_delta": false,
    "expiry": "15552000s",
    "ocsp_disable": false,
    "ocsp_expiry": "12h",
    "unified_crl": false,
    "unified_crl_on_existing_paths": false
  },
  "wrap_info": null,
  "warnings": null,
  "auth": null
}

list of keys
http://127.0.0.1:8200/v1/my-pki/keys?list=true

{
  "request_id": "1cf0833a-0f1c-95d7-7c6d-78dbe22d283e",
  "lease_id": "",
  "renewable": false,
  "lease_duration": 0,
  "data": {
    "key_info": {
      "3156cad1-2506-6bcd-ec6d-79ba4551818c": {
        "is_default": true,
        "key_name": "current-1681402889"
      }
    },
    "keys": [
      "3156cad1-2506-6bcd-ec6d-79ba4551818c"
    ]
  },
  "wrap_info": null,
  "warnings": null,
  "auth": null
}

Do issuers have key_id values in included in their entries as well?
vault read my-pki/issuer/default

ca_chain                          [... two certificates ...]
crl_distribution_points           []
issuer_id                         09a41e7f-9e25-9f99-6601-7e612636f2f7
issuer_name                       current-1681402889
issuing_certificates              []
key_id                            3156cad1-2506-6bcd-ec6d-79ba4551818c
leaf_not_after_behavior           err
manual_chain                      <nil>
ocsp_servers                      []
revocation_signature_algorithm    n/a
revoked                           false
usage                             issuing-certificates,ocsp-signing,read-only

Should the crl_distribution_points list have some entry here? In the vault UI we have a URL for "CRL Distribution Points" in the Configure PKI -> URLs tab.

What happens if you attempt to manually rebuild the CRL?
How should I rebuild the CRL?
Using the 'rotate' returns this response:

{
  "request_id": "3d1192f7-e00d-ac36-0540-2770e6c945ef",
  "lease_id": "",
  "renewable": false,
  "lease_duration": 0,
  "data": {
    "success": true
  },
  "wrap_info": null,
  "warnings": null,
  "auth": null
}

But it does not fix the issue. What I wonder is why the rotate does not print a single log entry, even with DEBUG enabled.

As for API LIST on my-pki/certs/revoked, I wonder why this is exposed in the API explorer if the endpoint was only added on 1.13? But anyway, I don't care about that one.

@maxb
Copy link
Contributor

maxb commented Apr 17, 2023

As for API LIST on my-pki/certs/revoked, I wonder why this is exposed in the API explorer if the endpoint was only added on 1.13?

A guess... maybe if you've been rapidly upgrading/downgrading versions, your browser had cached parts of the API explorer?

@maxb
Copy link
Contributor

maxb commented Apr 17, 2023

usage                             issuing-certificates,ocsp-signing,read-only

It appears your issuer is configured without crl-signing as an enabled Vault usage - https://developer.hashicorp.com/vault/api-docs/secret/pki#usage

Can you share your CA certificate, or just the value of its Extended Key Usage X509v3 extension? I'm wondering whether it isn't set up to permit CRL signing.

@flassman
Copy link
Author

flassman commented Apr 17, 2023

these are our X509v3 extensions:

        X509v3 extensions:
            X509v3 Basic Constraints:
                CA:TRUE
            Netscape Comment:
                OpenSSL Generated Certificate
            X509v3 Subject Key Identifier:
                A7:4B:95:AC:7B:97:35:20:31:DF:8A:B7:E1:49:EB:B8:CC:D3:F8:6B
            X509v3 Authority Key Identifier:
                keyid:FB:3D:5B:8B:12:98:F8:1C:30:EE:D2:17:34:13:15:4E:DA:2A:D3:23
            X509v3 Issuer Alternative Name:
                URI:http://some-path/cacert.pem
            X509v3 CRL Distribution Points:
                Full Name:
                  URI:http://some-path/crl.pem

so we do not explicitly permit CRL signing, but according to what I found here all usages should be allowed if no keyUsage extension is present:

If the keyUsage extension is present and marked critical, then it is used to enforce the usage of the certificate and key. The extension is used to limit the usage of a key; if the extension is not present or not critical, all types of usage are allowed.

@maxb
Copy link
Contributor

maxb commented Apr 17, 2023

That's a good point... all of the

(cert.KeyUsage&x509.KeyUsageCRLSign) == 0

style tests in the Vault PKI secrets engine will operate incorrectly with such a certificate.

@cipherboy
Copy link
Contributor

cipherboy commented Apr 17, 2023

Interesting, great investigation @maxb and @flassman.

Note that this is the same check as in Go: https://github.com/golang/go/blob/0853f8caec60f59df234c287be7f5971ab62133f/src/crypto/x509/x509.go#L2299

So while we could enable this ourselves, we'd need another bug report against Go as well before this will actually function, as otherwise you'd hit that error while building the CRL. :-)

Interestingly, the RFC is less clear than that RH link as it doesn't explicitly describe the behavior when missing:

Conforming CAs MUST include this extension in certificates that
contain public keys that are used to validate digital signatures on
other public key certificates or CRLs. When present, conforming CAs
SHOULD mark this extension as critical.

i.e., CA certs must include this, with the possible exception of a root CA. I do not see the subquotes all types, not present, not critical in RFC 5280 so I'm curious where they got that wording from...

Given this so far, unless we can find a more authoritative source and/or show OpenSSL and NSS both conform to RH's reading of this spec, I'm inclined to believe Go will reject our attempt to weaken restrictions on CRL building.

@maxb
Copy link
Contributor

maxb commented Apr 17, 2023

The oldest incarnation of that RFC, from 1999, lacks the requirement to include a key usage extension... however it was added in the 2002 incarnation, so there's an argument that by now, Go is being reasonable to enforce the requirement.

I guess the short term questions are now:

  • Why is Vault not surfacing a clearer diagnostic message?

  • How does Vault 1.10 and earlier successfully create a CRL for @flassman 's CA?

@maxb
Copy link
Contributor

maxb commented Apr 17, 2023

  • How does Vault 1.10 and earlier successfully create a CRL for @flassman 's CA?

Aha... the answer is that Vault 1.10 used a different legacy Go API that did not enforce that standard, which was deprecated by golang/go@5d47f87

So, as a side-effect of migrating to more modern underlying Go libraries, with other fixes, Vault >= 1.11 now no longer supports CRL generation for CAs without a modern compliant Key Usage extension.

@cipherboy
Copy link
Contributor

cipherboy commented Apr 17, 2023

Regarding the first bullet point, we could and should definitely add a log message here. We cannot err (in general -- we can and do from the config setting) for two reasons:

  1. This happens on issuer storage format migration, which would fail all reads of that issuer.
  2. If we left this behavior in (allowing CRLUsage when no KU is present), we would fail CRL building entirely for all CAs in a mount.

As it is now, we don't build one CRL as it lacks a crlSigning extension, but if you had another (valid) CA, you could successfully build its CRLs.

@maxb said:

So, as a side-effect of migrating to more modern underlying Go libraries, with other fixes, Vault >= 1.11 now no longer supports CRL generation for CAs without a modern compliant Key Usage extension.

Yes, this is a valid read and matches my understanding. I believe this was communicated in the CL at the time, as it also fixed a bug around CRL issuer name building when we upgraded to a newer Go version (#10550).

AFAIK, it is impossible to trigger this behavior with Vault-generated CAs; this behavior would strictly be from an imported CA. We could potentially err on this import on newer versions of Vault, but note that signing is not affected, only CRL generation (including external CRL generation capabilities -- since the key material was already outside of Vault though, you could manually maintain an external CRL without calling into Vault for the signing operation). I think erring on 1.10->1.11+ migration would be a step too far though. It is not immediately clear if clients will reject this CRL though.


@flassman One workaround could be to re-generate this CA (with same key material) and the new KU extension; if it is a root CA, you can import it into Vault and set it as non-default as long as you had the same SKID/AKID/Subject: your issuance would use the old root (might need a manual_chain entry though to prevent from exposing this new root) -- and CRL rebuilding would be done under the new "equivalent" root. Again, subject to the "not clear if clients will reject this CRL due to its apparent issuer lacking CRL Signing KU bits" though.

@maxb
Copy link
Contributor

maxb commented Apr 17, 2023

  • Why is Vault not surfacing a clearer diagnostic message?

Turns out to be because this is a "do this for all issuers" operation, and it is "successfully" building no CRL at all, having reviewed all issuers and determined none of them have CRL signing turned on as a usage.

Regarding the first bullet point, we could and should definitely add a log message here.

Another option to consider, would be to change the current rather bare bones response from the crl/rotate endpoint:

"data": {
"success": true
},

to give a bit more information about what happened - and possibly a warning if no CRL was actually built.

@flassman
Copy link
Author

flassman commented Apr 17, 2023

  • How does Vault 1.10 and earlier successfully create a CRL for @flassman 's CA?

Aha... the answer is that Vault 1.10 used a different legacy Go API that did not enforce that standard, which was deprecated by golang/go@5d47f87

So, as a side-effect of migrating to more modern underlying Go libraries, with other fixes, Vault >= 1.11 now no longer supports CRL generation for CAs without a modern compliant Key Usage extension.

Thanks to you all for your fast answers and clear analysis of my issue!

@cipherboy
Copy link
Contributor

cipherboy commented Apr 17, 2023

@maxb Aha, past me had the same comment on Slack:

Per ITU X.509, I'm going to loosen our crlSigning check:

keyUsage matches if all of the bits set in the presented value are also set in the key usage extension in the stored attribute value, or if there is no key usage extension in the stored attribute value

And then:

Ah I guess it doesn't matter lol:

* 1 error occurred:
  * error building CRLs: unable to build CRL for issuer (f8f04fa1-9d8b-c77e-a227-d390ae652a1e): error creating new CRL: x509: issuer must have the crlSign key usage bit set

Notably, the latest ITU X.509 specification on printed page 73/pdf page 83 still includes this language. So I think we do have justification to go back to Go and get them to loosen the check.

@cipherboy
Copy link
Contributor

I've filed golang/go#59669 to discuss this with Go -- we've come up with a workaround (also mentioned in golang/go#49414) that we could assert KeyUsage manually on this certificate prior to the CRL generation call.

We're still discussing the short-term workarounds we'd like to do in the mean time, however.

@cipherboy
Copy link
Contributor

@maxb @flassman I've opened a PR which should add the warning infrastructure you talked about above, Max. It was a bit more involved than I was hoping, but its done now so 😆

cipherboy added a commit that referenced this issue Apr 19, 2023
When an entire issuer equivalency class is missing CRL signing usage
(but otherwise has key material present), we should add a warning so
operators can either correct this issuer or create an equivalent version
with KU specified.

Resolves: #20137

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy added a commit that referenced this issue Apr 19, 2023
…suer equivalency sets (#20253)

* Add infrastructure for warnings on CRL rebuilds

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add warning on issuer missing KU for CRL Signing

When an entire issuer equivalency class is missing CRL signing usage
(but otherwise has key material present), we should add a warning so
operators can either correct this issuer or create an equivalent version
with KU specified.

Resolves: #20137

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add tests for issuer warnings

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add changelog entry

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Fix return order of CRL builders

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

---------

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants