-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Support Y10K value in notAfter field when signing non-CA certificates #13736
Conversation
Hi @gsharris! Just a note to please sign the CLA. The bot checks to see that your GitHub handle and your email address match, depending on how you submitted the PR, as well as if your "organization" is publicly viewable, in the case of Corporate CLAs. If you could also add a changelog entry, that'd be appreciated too! |
Hi @hsimon-hashicorp I have signed the CLA and resolved the merge conflicts. This PR is ready for review, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gsharris! Just one thing: can you add a unit test to verify the new field works with leaf certificates? If you'd like an example to refer to, you can look at this test from the previous PR: https://github.com/hashicorp/vault/pull/12795/files#diff-b2444759cbec0bce772f8da68de238c7cb6e6f065c29b5ab05880124f0ede220R229.
Hi, I have added the requested test for using the not_after value for non-CA certificates when using the sign operation (the test for the role operation was already there from the prior PR you referenced). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry to bother but I realized the check's failing due to changelog entry. CHANGELOG.md
is mostly auto-generated, you'd just need to drop an entry in changelog/13736.txt
with value similar to 12795.txt, but with your new entry.
Edit And you added it right as I mentioned it. :-D But I'd still revert the changes to CHANGELOG.md
IMO.
@cipherboy Yes, I saw that job failing right when you said that ;) I have the changelog cleaned up now. |
Thank you for your contribution @gsharris :) |
Issue #12795 supports the Y10K value through a new "not_after" field to be compliant with the IEEE 802.1AR-2018 standard. The existing PR supports the not_after field when calling the API to generate CA certificates, but is not supported when signing non-CA certificates. This PR enhances the new feature to support signing non-CA certificates through use of the not_after field the same way the existing PR supports signing CA certificates with the not_after field.