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

Implemented device certificate with Y10K expiry for certificate by providing the not_after field #12795

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

skhilar
Copy link
Contributor

@skhilar skhilar commented Oct 11, 2021

fixes #12157

As per the IEE 802.1AR-2018 standard, the notAfter field of the IDevID certificate should use the Generalized Time value 99991231235959Z. However, in the PKI secrets engine currently, the max ttl value that can be specified is ~290 years.

Support an additional device_flag in certificate roles, which, when set ignores the ttl field and sets the notAfter value to Y10K.

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 11, 2021

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – vault-storybook October 11, 2021 11:46 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 11, 2021 11:46 Inactive
@skhilar skhilar changed the title Implemented device certificate with Y10K expiry by providing the device Implemented device certificate with Y10K expiry by providing the device_flag in role and setting ieee_profile for CA Oct 11, 2021
@hghaf099
Copy link
Contributor

Hi @skhilar, before we can review this, please be sure to sign the CLA. Thanks!

@hghaf099
Copy link
Contributor

we also need a changelog for this PR. Please follow this link for instructions https://github.com/hashicorp/vault/blob/main/CHANGELOG.md

@skhilar
Copy link
Contributor Author

skhilar commented Oct 15, 2021

Hi @skhilar, before we can review this, please be sure to sign the CLA. Thanks!

Hi @hghaf099 I have signed the CLA. Still that is not reflected in the PR.

@cipherboy
Copy link
Contributor

cipherboy commented Oct 15, 2021

@skhilar It looks like the email used to author the commit is different than the one in your profile. You might want to add that email to your GitHub profile (maybe you'll need to resign the CLA in that case?), or update the commit to link to your mail (git config --global user.email ... and git commit --amend --reset-author to update the commit). You might be able to use a GitHub no-reply email if you'd prefer.

@vercel vercel bot temporarily deployed to Preview – vault October 18, 2021 03:07 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 18, 2021 03:07 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 18, 2021 03:16 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 18, 2021 03:16 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 18, 2021 03:50 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 18, 2021 03:50 Inactive
Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this updated for the CLA, @skhilar.

I talked with @sgmiller about this and I think we'd both prefer if this didn't introduce any new flags, but changed the behavior of the TTL flag. What if we change TTL as follows:

			"ttl": {
				Type: framework.TypeDurationSecond,
				Description: `The lease duration if no specific lease duration is
requested. The lease duration controls the expiration
of certificates issued by this backend. Defaults to
the value of max_ttl. If value exceeds 10,000 years
(3600 * 24 * 365 * 1000 seconds), the certificate's
notAfter field will be set to the IEEE 802.1AR-2018
standard's Y10K value (9999-12-31T23:59:59Z).`,
				DisplayAttrs: &framework.DisplayAttributes{
					Name: "TTL",
				},
			},

Then in cert_util.go, you can detect a large value and update notAfter appropriately. I think that'd be a cleaner approach, rather than introducing two new flags that are (presently) mostly duplicates.

Thoughts? Are there other values of IEEE standard with different notAfter times that might be useful?

Another alternative we liked was introducing a proper notAfter field as an alternative to the TTL field.

Let us know what you think :)

builtin/logical/pki/path_roles.go Outdated Show resolved Hide resolved
},
"ieee_profile": {
Type: framework.TypeString,
Description: `Set notAfter field to Y10K to compliant with IEEE 802.1AR-2018 standard for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Description: `Set notAfter field to Y10K to compliant with IEEE 802.1AR-2018 standard for
Description: `Set notAfter field to Y10K to comply with IEEE 802.1AR-2018 standard for

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not relevant if review suggestion is taken about TTL).

Copy link
Contributor Author

@skhilar skhilar Oct 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added not_after field which accepts date value in UTC with format YYYY-MM-ddTHH:MM:SSZ. Same value will be set as notAfter field for the certificate as an alternative to ttl.

builtin/logical/pki/path_roles.go Outdated Show resolved Hide resolved
builtin/logical/pki/path_roles.go Outdated Show resolved Hide resolved
builtin/logical/pki/cert_util.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault October 22, 2021 08:26 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 22, 2021 08:26 Inactive
@skhilar skhilar changed the title Implemented device certificate with Y10K expiry by providing the device_flag in role and setting ieee_profile for CA Implemented device certificate with Y10K expiry for certificate by providing the not_after field Oct 22, 2021
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 22, 2021 09:30 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 22, 2021 09:30 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 22, 2021 09:38 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 22, 2021 09:38 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 22, 2021 10:11 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 22, 2021 10:11 Inactive
@skhilar
Copy link
Contributor Author

skhilar commented Oct 28, 2021

I have raised PR introducing "not_after" field. @sgmiller and @cipherboy please review and provide your feedback.

@vercel vercel bot temporarily deployed to Preview – vault October 31, 2021 03:24 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 31, 2021 03:24 Inactive
@skhilar
Copy link
Contributor Author

skhilar commented Oct 31, 2021

@sgmiller and @cipherboy thank you very much for the feedback. I have added a validation for the same. Both "ttl" and "not_after" should not be present in the same request. Please review the code and provide your feedback.

@skhilar
Copy link
Contributor Author

skhilar commented Nov 4, 2021

@sgmiller and @cipherboy did you get chance to review the code? I have added the validation for TTL and NotAfter. Please review the code and provide your comments.

@cipherboy
Copy link
Contributor

@sgmiller this looks good to me -- what about you?

Copy link
Collaborator

@sgmiller sgmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor ask.

}
if resp == nil {
t.Fatal("expected ca info")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these tests, can we retrieve the resulting cert from the CA endpoint and validate the correct NotAfter value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgmiller I have addressed the review comments. Please have a look.

builtin/logical/pki/fields.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault November 5, 2021 15:53 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 5, 2021 15:53 Inactive
@vercel vercel bot temporarily deployed to Preview – vault November 5, 2021 16:02 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook November 5, 2021 16:02 Inactive
Copy link
Contributor

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, @skhilar!

@cipherboy cipherboy merged commit 56e81b8 into hashicorp:main Nov 11, 2021
@@ -0,0 +1,3 @@
```release-note:feature
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use a particular changelog format for new features. I think this is more of an improvement, so I moved it to that section. @cipherboy I'm going to tag you on an enterprise PR if you don't mind giving it a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Y10K value in notAfter field to be compliant with IEEE 802.1AR-2018 standard
6 participants