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

Check cnf claim with CSR or SSH public key fingerprint #1660

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maraino
Copy link
Contributor

@maraino maraino commented Dec 29, 2023

This commit allows tying tokens with the provided CSR or SSH public key. Tokens with a confirmation claim kid (cnf.kid) will validate that the provided fingerprint (kid) matches the CSR or SSH public key.

This check will only be present in JWK and X5C provisioners.

Fixes #1637

Related PRs:

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Dec 29, 2023
This commit allows tying tokens with the provided  CSR or SSH public
key. Tokens with a confirmation claim kid (cnf.kid) will validate that
the provided fingerprint (kid) matches the CSR or SSH public key.

This check will only be present in JWK and X5C provisioners.

Fixes #1637
@maraino maraino marked this pull request as ready for review January 5, 2024 23:47
@maraino maraino requested a review from hslatman January 5, 2024 23:47
@hslatman hslatman added this to the v0.26.0 milestone Mar 26, 2024
@hslatman hslatman modified the milestones: v0.26.0, v0.26.1 Mar 29, 2024
Comment on lines +499 to +501
// fingerprintValidator is a CertificateRequestValidator that checks the
// fingerprint of the certificate with the provided one.
type fingerprintValidator string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// fingerprintValidator is a CertificateRequestValidator that checks the
// fingerprint of the certificate with the provided one.
type fingerprintValidator string
// fingerprintValidator is a CertificateRequestValidator that checks the
// fingerprint of the certificate request with the provided one.
type fingerprintValidator string

Copy link
Member

Choose a reason for hiding this comment

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

Also, to clarify what it's operating on, maybe this can be named csrFingerprintValidator or certificateRequestFingerprintValidator.

Comment on lines +432 to +437
// sshFingerprintValidator is a SSHPublicKeyValidator that checks the
// fingerprint of the public key with the provided one.
type sshFingerprintValidator string

func (s sshFingerprintValidator) Valid(key ssh.PublicKey) error {
if s != "" {
Copy link
Member

@hslatman hslatman Apr 2, 2024

Choose a reason for hiding this comment

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

It's a little surprising to see the SSH fingerprint being a slightly different thing than an X509 fingerprint. I know we're using fingerprint for both full certificate hashes as well as derivations of just the public key in several places already, but in this case different logic seems to be used for a single functionality, whereas for the other places we refer to fingerprints, the context makes usage more clear.

IMO it would be better if we were to operate on the canonical contents of an SSH certificate request instead, so that the semantics are more similar for these checks. That would also keep the option open for an actual public key equality validator. It would require us to define that canonical format, because I think there's no existing definition for it, except for the final certificates.

IMO it's an option to skip (temporarily, or indefinitely) the SSH part of this PR if we don't find a good way for the SSH certificate request representation. I think that's preferred over having this logic with different semantics.

@hslatman hslatman modified the milestones: v0.26.1, v0.26.2 Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to bind JWK provisioner tokens with CSR
2 participants