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

Add permanent identifier in the CreateAttestation response #204

Merged
merged 4 commits into from
Mar 25, 2023

Conversation

maraino
Copy link
Contributor

@maraino maraino commented Mar 24, 2023

This commit adds an optional permanent identifier in the CreateAttestation response. In the YubiKey KMS, the permanent identifier is the serial number in the attestation certificate.

This PR also files all linter errors.

This commit adds an optional permanent identifier in the response of
CreateAttestation request. In the YubiKey KMS the permanent identifier
is the serial number present in the attestation certificate.
@maraino maraino requested a review from hslatman March 24, 2023 21:20
Copy link
Member

@hslatman hslatman left a comment

Choose a reason for hiding this comment

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

Looks OK for the most part. Only reserve I have is if this should be called PermanentIdentifier, or something slightly more generic? While it's certainly used like a PermanentIdentifier would be used, this isn't currently coming from an actual PermanentIdentifier SAN, nor do we encode the Assigner with it (although the Issuer of the attestation cert can be used for that in lieu of the Assigner). It's a great name for it is and does, though. Some alternatives would be PersistentIdentifier and DurableIdentifier, but I still like PermanentIdentifier better. Identifier feels too generic and doesn't make clear that it can't/won't change, so also not great.

kms/yubikey/yubikey.go Outdated Show resolved Hide resolved
Co-authored-by: Herman Slatman <hslatman@users.noreply.github.com>
@maraino maraino requested a review from hslatman March 25, 2023 00:31
@maraino maraino merged commit 54a1c86 into master Mar 25, 2023
@maraino maraino deleted the permanent-identifier branch March 25, 2023 00:42
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.

2 participants