Expand NotAfter limit enforcement behavior #15152
Conversation
|
Can be tested with: source devvault
vault secrets enable pki
vault write pki/root/generate/internal common_name="root example.com" issuer_name=root ttl=15s
vault write /pki/roles/local-testing allow_any_name=true enforce_hostnames=false no_store=true
sleep 16s # let the root expire
vault write pki/issue/local-testing common_name=hello # should err
vault write pki/issuer/root issuer_name=root leaf_not_after_behavior=permit
vault write pki/issue/local-testing common_name=hello # should be default ttl of a month or so
vault write pki/issuer/root issuer_name=root leaf_not_after_behavior=truncate
vault write pki/issue/local-testing common_name=hello # should be expired
vault write pki/issuer/root issuer_name=root leaf_not_after_behavior=err
vault write pki/issue/local-testing common_name=hello # should err |
cipherboy
force-pushed
the
cipherboy-remove-ttl-issuance-restrictions
branch
2 times, most recently
from
a83f5f5
to
b1dab4f
Compare
stevendpclark
force-pushed
the
stevendpclark/vault-5748-read-legacy-on-secondary
branch
from
19a2981
to
0c53f24
Compare
cipherboy
force-pushed
the
cipherboy-remove-ttl-issuance-restrictions
branch
from
b1dab4f
to
cd3229a
Compare
| @@ -150,21 +150,27 @@ func setLegacyBundleMigrationLog(ctx context.Context, s logical.Storage, lbm *le | |||
| return s.Put(ctx, json) | |||
| } | |||
|
|
|||
| func getLegacyCertBundle(ctx context.Context, s logical.Storage) (*certutil.CertBundle, error) { | |||
| func getLegacyCertBundle(ctx context.Context, s logical.Storage) (*issuerEntry, *certutil.CertBundle, error) { | |||
There was a problem hiding this comment.
nit?: The migration works properly, matches with legacy behaviour, due to the iota definition of NotAfterBehavior the value ErrNotAfterBehavior comes first. Should we either be explicit within the migration perhaps of the value we want or test that ErrNotAfterBehavior. == 0?
There was a problem hiding this comment.
Would ImportIssuer... be the correct place to set this explicitly? Not sure what makes import on migration different than others, but yes, zero value being err was intentional :-)
There was a problem hiding this comment.
Well the others to my knowledge have the value set through either the default on the parameter definition or end-user values, this one really is just up to the iota. I don't think we can put it within importIssuer as there are so many end-users of that function.
How about just adding a check within storage_migrations_test.go that a migrated issuer has a value of ErrNotAfterBehavior set? I'd just want something to break if that iota accidently gets re-ordered.
There was a problem hiding this comment.
importIssuer is already opinionated though, in other forms: we automatically construct a chain (rather than giving the option to specify a manual chain at import time), and when calling e.g., /config/ca, we allow importing multiple issuers, and so there's really no way to control name either.
There was a problem hiding this comment.
Updated with tests and the updated ImportIssuer behavior :)
There was a problem hiding this comment.
Good points, let's go with adding it to importIssuer.
Vault previously strictly enforced NotAfter/ttl values on certificate requests, erring if the requested TTL extended past the NotAfter date of the issuer. In the event of issuing an intermediate, this behavior was ignored, instead permitting the issuance. Users generally do not think to check their issuer's NotAfter date when requesting a certificate; thus this behavior was generally surprising. Per RFC 5280 however, issuers need to maintain status information throughout the life cycle of the issued cert. If this leaf cert were to be issued for a longer duration than the parent issuer, the CA must still maintain revocation information past its expiration. Thus, we add an option to the issuer to change the desired behavior: - err, to err out, - permit, to permit the longer NotAfter date, or - truncate, to silently truncate the expiration to the issuer's NotAfter date. Since expiration of certificates in the system's trust store are not generally validated (when validating an arbitrary leaf, e.g., during TLS validation), permit should generally only be used in that case. However, browsers usually validate intermediate's validity periods, and thus truncate should likely be used (as with permit, the leaf's chain will not validate towards the end of the issuance period). Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
cipherboy
force-pushed
the
cipherboy-remove-ttl-issuance-restrictions
branch
from
cd3229a
to
a12457f
Compare
|
Thanks @stevendpclark! |
Vault previously strictly enforced
NotAfter/ttlvalues on certificaterequests, erring if the requested TTL extended past the
NotAfterdate ofthe issuer. In the event of issuing an intermediate, this behavior was
ignored, instead permitting the issuance.
Users generally do not think to check their issuer's
NotAfterdate whenrequesting a certificate; thus this behavior was generally surprising.
Per RFC 5280 however, issuers need to maintain status information
throughout the life cycle of the issued cert. If this leaf cert were to
be issued for a longer duration than the parent issuer, the CA must
still maintain revocation information past its expiration.
Thus, we add an option to the issuer to change the desired behavior:
err, to err out,permit, to permit the longerNotAfterdate, ortruncate, to silently truncate the expiration to the issuer'sNotAfterdate.Since expiration of certificates in the system's trust store are not
generally validated (when validating an arbitrary leaf, e.g., during TLS
validation), permit should generally only be used in that case. However,
browsers usually validate intermediate's validity periods, and thus
truncate should likely be used (as with permit, the leaf's chain will
not validate towards the end of the issuance period).
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>