Add support for verifying server certs against SPIFFE IDs #252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds support to "certigo connect" to verify that the server presented a certificate for a specific hostname or SPIFFE ID. This name can now be unrelated to the hostname to connect to and the Server Name Indication (SNI) to send to the server during the TLS handshake. Prior to this commit, the name expected in the certificate could only be a hostname, specified either via --name (SNI) or the hostname to connect to. SPIFFE IDs -- URLs with "spiffe" as the scheme -- are not useful as hostnames or SNIs. As a result, this commit adds a new --expected-name switch to certigo connect. This override provides full control over what name to expect in the server certificate presented during the TLS handshake. For example, this name can now be different from the hostname connected to and the name specified in Server Name Indication (SNI). Moreover, if --expected-name is a URL with "spiffe" scheme, it is only matched against Subject Alternative Names of type URI, as per the SPIFFE spec.
|
|
jdtw
reviewed
Jan 10, 2022
lib/verify.go
Outdated
| spiffeIDExpected := strings.HasPrefix(expectedName, "spiffe://") | ||
| var expectedDnsName string | ||
| if spiffeIDExpected { | ||
| expectedDnsName = "" |
There was a problem hiding this comment.
This case can be omitted, since it's a no-op right now?
var expectedDNSName string
if !spiffeIDExpected {
expectedDnsName = expectedName
}
lib/verify.go
Outdated
| // SPIFFE ID. We thus perform this check explicitly here. | ||
| expectedSpiffeID := expectedName | ||
| matched := false | ||
| for _, uri := range certs[0].URIs { |
There was a problem hiding this comment.
It's invalid for a SPIFFE cert to have more than one URI SAN. Should we warn in this case?
There was a problem hiding this comment.
I'm going with failing the match when there's not exactly one URI SAN, as per X.509 SPIFFE SVID spec.
cli/cli.go
Outdated
| hostname = *connectName | ||
| // Determine what name the server's certificate should match | ||
| var expectedNameInCertificate string | ||
| if *connectVerifyExpectedName != "" { |
There was a problem hiding this comment.
Optional. I prefer a switch in these situations:
switch {
case *connectVerifyExpectedName != "":
expectedNameInCertificate = *connectVerifyExpectedName
case *connectName != "":
expectedNameInCertificate = *connectName
default:
expectedNameInCertificate = strings.Split(*connectTo, ":")[0]
}
There was a problem hiding this comment.
oooh. That's nice! done.
lib/verify.go
Outdated
| // expectedName could be a hostname or could be a SPIFFE ID (spiffe://...) | ||
| // x509 package doesn't support verifying SPIFFE IDs. We thus have to do a bit more work here. | ||
| spiffeIDExpected := strings.HasPrefix(expectedName, "spiffe://") | ||
| var expectedDnsName string |
There was a problem hiding this comment.
nit: this should be expectedDNSName, since Go likes keeping acronym casing the same (DNS instead of Dns)
As per SPIFFE spec: * Reject certificates with multiple URI SANs when expected name is a SPIFFE ID * Case-insensitive protocol/scheme check of URI SANs when expected name is a SPIFFE ID
jdtw
approved these changes
Jan 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds support to "certigo connect" to verify that the
server presented a certificate for a specific hostname or
SPIFFE ID. This name can now be unrelated to the hostname
to connect to and the Server Name Indication (SNI) to send
to the server during the TLS handshake.
Prior to this commit, the name expected in the certificate
could only be a hostname, specified either via --name (SNI)
or the hostname to connect to. SPIFFE IDs -- URLs with
"spiffe" as the scheme -- are not useful as hostnames or
SNIs. As a result, this commit adds a new --expected-name
switch to certigo connect. This override provides full
control over what name to expect in the server certificate
presented during the TLS handshake. For example, this name
can now be different from the hostname connected to and the
name specified in Server Name Indication (SNI). Moreover,
if --expected-name is a URL with "spiffe" scheme, it is
only matched against Subject Alternative Names of type URI,
as per the SPIFFE spec.