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

internal/credentials: fix a bug and add one more helper function SPIFFEIDFromCert #3929

Merged
merged 4 commits into from Oct 9, 2020

Conversation

ZhenLian
Copy link
Contributor

@ZhenLian ZhenLian commented Oct 6, 2020

The main purpose of this PR is to fix an issue I found when working on some integration tests of advancedtls: when checking the SPIFFE ID of the real certificate, we shouldn't check len(uri.RawPath) == 0 since RawPath will always be empty. Path is the right one to use. I also added a test to parse a real certificate with SPIFFE ID in it.
Besides, I added another helper function SPIFFEIDFromCert that will do mostly the same as SPIFFEIDFromState, except that it parses the certificate directly. This will be used by advancedtls to get the SPIFFE ID at the time doing custom authorization(at that time, we can get the certificate, but the tls.ConnectionState is not ready yet).
@easwars @dfawley
Feel free to let me know if you have any questions. Thank you so much!

@ZhenLian ZhenLian requested a review from dfawley October 6, 2020 18:14
@@ -57,7 +67,7 @@ func SPIFFEIDFromState(state tls.ConnectionState) *url.URL {
return nil
}
// A valid SPIFFE certificate can only have exactly one URI SAN field.
if len(state.PeerCertificates[0].URIs) > 1 {
if len(cert.URIs) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be checked outside of the for loop? (probably alongside the nil check)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we move it outside, the logging can't be moved together because at that time we are not sure the user is using SPIFFE ID yet. In current implementation, we will only log the warning under the condition uri.Scheme == "spiffe"(which indicates the user uses SPIFFE ID).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear if a valid SPIFFE certificate can only have one URI SAN field of any scheme or it can have multiple URI SAN fields, but only one of them can have a scheme of spiffe. If its the latter, then the current check does not do the correct thing. It its the former, then you can check at the top before the for loop.

internal/credentials/spiffe_test.go Outdated Show resolved Hide resolved
internal/credentials/spiffe_test.go Outdated Show resolved Hide resolved
internal/credentials/spiffe_test.go Outdated Show resolved Hide resolved
internal/credentials/spiffe_test.go Outdated Show resolved Hide resolved
@easwars easwars added this to the 1.33 Release milestone Oct 6, 2020
@ZhenLian
Copy link
Contributor Author

ZhenLian commented Oct 7, 2020

@easwars Thanks for the review! I've updated the code based on your comments. Please let me know if you see any other issues. Thanks!

@@ -57,7 +67,7 @@ func SPIFFEIDFromState(state tls.ConnectionState) *url.URL {
return nil
}
// A valid SPIFFE certificate can only have exactly one URI SAN field.
if len(state.PeerCertificates[0].URIs) > 1 {
if len(cert.URIs) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear if a valid SPIFFE certificate can only have one URI SAN field of any scheme or it can have multiple URI SAN fields, but only one of them can have a scheme of spiffe. If its the latter, then the current check does not do the correct thing. It its the former, then you can check at the top before the for loop.

internal/credentials/spiffe_test.go Outdated Show resolved Hide resolved
@ZhenLian
Copy link
Contributor Author

ZhenLian commented Oct 8, 2020

I'm not clear if a valid SPIFFE certificate can only have one URI SAN field of any scheme or it can have multiple URI SAN fields, but only one of them can have a scheme of spiffe. If its the latter, then the current check does not do the correct thing. It its the former, then you can check at the top before the for loop.

According to SPIFFE standards, "An X.509 SVID MUST contain exactly one URI SAN".
I might be wrong, but I think the issue here is the logging. If we move

if len(cert.URIs) > 1 {
    logger.Warning("invalid SPIFFE ID: multiple URI SANs")
    return nil
}

outside of the for loop, users using non-SPIFFE ID cert might also get the warning log(e.g. when they have https://foo.com and http://bar.com in their cert, this is valid, because they don't intend to use SPIFFE, but they will also get the warning if we move the code outside the loop). Do you have any ideas on how to solve this? Thanks!

@ZhenLian ZhenLian requested a review from easwars October 8, 2020 06:44
@dfawley dfawley assigned easwars and unassigned ZhenLian Oct 8, 2020
@easwars
Copy link
Contributor

easwars commented Oct 9, 2020

LGTM.

@easwars easwars assigned ZhenLian and unassigned easwars Oct 9, 2020
@ZhenLian
Copy link
Contributor Author

ZhenLian commented Oct 9, 2020

@easwars Thank you so much for the review!

@ZhenLian ZhenLian merged commit 84e85f7 into grpc:master Oct 9, 2020
@dfawley dfawley removed this from the 1.34 Release milestone Nov 6, 2020
GarrettGutierrez1 pushed a commit that referenced this pull request Nov 6, 2020
…FEIDFromCert (#3929)

* internal/credentials: fix a bug and add one more helper function
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants