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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from Oct 9, 2020
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c4efd1c
internal/credentials: fix a bug and add one more helper function
ZhenLian Oct 6, 2020
d09c144
update create.sh
ZhenLian Oct 6, 2020
9a3cd59
fix first round of review
ZhenLian Oct 7, 2020
8038605
change expectID to wantID
ZhenLian Oct 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 13 additions & 3 deletions internal/credentials/spiffe.go
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ package credentials

import (
"crypto/tls"
"crypto/x509"
"net/url"

"google.golang.org/grpc/grpclog"
Expand All @@ -38,8 +39,17 @@ func SPIFFEIDFromState(state tls.ConnectionState) *url.URL {
if len(state.PeerCertificates) == 0 || len(state.PeerCertificates[0].URIs) == 0 {
return nil
}
return SPIFFEIDFromCert(state.PeerCertificates[0])
}

// SPIFFEIDFromCert parses the SPIFFE ID from x509.Certificate. If the SPIFFE
// ID format is invalid, return nil with warning.
func SPIFFEIDFromCert(cert *x509.Certificate) *url.URL {
if cert == nil {
return nil
}
var spiffeID *url.URL
for _, uri := range state.PeerCertificates[0].URIs {
for _, uri := range cert.URIs {
if uri == nil || uri.Scheme != "spiffe" || uri.Opaque != "" || (uri.User != nil && uri.User.Username() != "") {
continue
}
Expand All @@ -48,7 +58,7 @@ func SPIFFEIDFromState(state tls.ConnectionState) *url.URL {
logger.Warning("invalid SPIFFE ID: total ID length larger than 2048 bytes")
return nil
}
if len(uri.Host) == 0 || len(uri.RawPath) == 0 || len(uri.Path) == 0 {
if len(uri.Host) == 0 || len(uri.Path) == 0 {
logger.Warning("invalid SPIFFE ID: domain or workload ID is empty")
return nil
}
Expand All @@ -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.

logger.Warning("invalid SPIFFE ID: multiple URI SANs")
return nil
}
Expand Down
25 changes: 25 additions & 0 deletions internal/credentials/spiffe_test.go
View file
Open in desktop
Expand Up @@ -21,10 +21,13 @@ package credentials
import (
"crypto/tls"
"crypto/x509"
"encoding/pem"
"io/ioutil"
"net/url"
"testing"

"google.golang.org/grpc/internal/grpctest"
"google.golang.org/grpc/testdata"
)

type s struct {
Expand Down Expand Up @@ -178,3 +181,25 @@ func (s) TestSPIFFEIDFromState(t *testing.T) {
})
}
}

func (s) TestSPIFFEIDFromCert(t *testing.T) {
data, err := ioutil.ReadFile(testdata.Path("x509/spiffe_cert.pem"))
if err != nil {
t.Fatalf("Failed to load the test credentials: %v", err)
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
}
block, rest := pem.Decode(data)
if len(rest) != 0 {
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
t.Fatalf("Failed to parse the certificate: invalid format")
}
cert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
t.Fatalf("Failed to load the test credentials: %v", err)
}
uri := SPIFFEIDFromCert(cert)
if uri == nil {
t.Fatalf("SPIFFE ID is nil")
}
if uri.String() != "spiffe://foo.bar.com/client/workload/1" {
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
t.Fatalf("SPIFFE ID not expected, got %s, want %s", uri.String(), "spiffe://foo.bar.com/client/workload/1")
}
}
ZhenLian marked this conversation as resolved.
Show resolved Hide resolved
10 changes: 10 additions & 0 deletions testdata/x509/create.sh
View file
Open in desktop
Expand Up @@ -100,5 +100,15 @@ openssl x509 -req \
-extensions test_client
openssl verify -verbose -CAfile client_ca_cert.pem client2_cert.pem

# Generate a cert with SPIFFE ID.
openssl req -x509 \
-newkey rsa:4096 \
-keyout spiffe_key.pem \
-out spiffe_cert.pem \
-nodes \
-days 3650 \
-subj /C=US/ST=CA/L=SVL/O=gRPC/CN=test-client1/ \
-addext "subjectAltName = URI:spiffe://foo.bar.com/client/workload/1"

# Cleanup the CSRs.
rm *_csr.pem
33 changes: 33 additions & 0 deletions testdata/x509/spiffe_cert.pem
View file
Open in desktop
@@ -0,0 +1,33 @@
-----BEGIN CERTIFICATE-----
MIIFsjCCA5qgAwIBAgIUPxiyjwxoDyMeRvl4g9TSdvLlCA0wDQYJKoZIhvcNAQEL
BQAwTjELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkNBMQwwCgYDVQQHDANTVkwxDTAL
BgNVBAoMBGdSUEMxFTATBgNVBAMMDHRlc3QtY2xpZW50MTAeFw0yMDEwMDYxNzI2
MzFaFw0zMDEwMDQxNzI2MzFaME4xCzAJBgNVBAYTAlVTMQswCQYDVQQIDAJDQTEM
MAoGA1UEBwwDU1ZMMQ0wCwYDVQQKDARnUlBDMRUwEwYDVQQDDAx0ZXN0LWNsaWVu
dDEwggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoICAQCV84YR/EV55qfFynHh
QvWEZW5hUI9q0DeD5kG5CarrkOj11rZuQIBZ7X23CJbeoVbrvbYLghsPYJzxS/n3
Qlwwzb5k+L0Qt+HrBD836HcSK5k1oh0jGGMaGownap+XCZH9g52s/8iiwfI02CmN
TbwsNp7wtSEFgNOd2OlzhT6wBLF2Q6uxfBmsDpiChxe2Fs1lyan9RH8fYEf7sxwP
E+SgBfEs7dSG5ZwFfdF+pd1T3IfrVjIxechKO1MO7HTSxbOTj6eHf1NeErDTGPA7
VrnDCupRgcDGyAhFd54r62R8TbTjn5MwzMxElO45Ck/Ej7Qw/GWeaBHj/dMa6mhE
R55PvnKuyj+k9t0Rf0HDZyONtY5/OLqI/xVr27Y1o9v5FysNgjWPkZMRpvuCzkeC
2RuE6k2TfBDRLiCyYu/Zzw+ZtUyTAKtWtefLdQBjrYpnhrDPpmrnTWomX/e9pylE
WfkyxCswiPnDw7ypI7uFSTkz0+bUaROmAtlPvR+3SjaQDWigwz3eJsdIaeg5AY9q
//rWaal6l2iR0Ou9L6A9lLxh5iN/ch+OGk4QPK6pFbOy3IqYfmQ+IpAXG0da9RT2
EN76cNa3bldEjRRON8oQ3HZmhOQJqVxhQciUz84sTjAqH8WvqqbdG9HKUoZ19T5Z
9vNldjlQn33Mi5gBxdugqdnmCQIDAQABo4GHMIGEMB0GA1UdDgQWBBT8rr0kPapk
bGLJ4EU1582sw7WlOTAfBgNVHSMEGDAWgBT8rr0kPapkbGLJ4EU1582sw7WlOTAP
BgNVHRMBAf8EBTADAQH/MDEGA1UdEQQqMCiGJnNwaWZmZTovL2Zvby5iYXIuY29t
L2NsaWVudC93b3JrbG9hZC8xMA0GCSqGSIb3DQEBCwUAA4ICAQA15Ne+Lz5cN1/B
fkys4QHDWJ0n5Zy9OtwSW6aTyqIIwls6OOSkJn3qJMoT2oFvoHoOxb0swyN+zUoD
pmPEd7FHkMm8BhRqoyH3UZGR7kOSIIcfvldVZbW9mD88A04qvLsWkkanMyGhkYV4
0TXyb8USdjeNm1H32iF4k24czSpvoOYo9HOQv+4aFcqTMnGwS7CvwU6O6vVU8gIy
HYP/oWnkhap6X7acjPxYoW5IDZdN9vdMz9wQlKlc799lWqOCuwl68NSuTNcNNFyn
TXfFWZaghb7iXsUezGYTY9glsPxY0Egmbcmxut0gz0U2BNVvNGKUUu55MlAS7yXO
Y7eTfSSf6DJesFQKwTg8qlyNLjzbLSmhvz6EPV55ToUxPPA9CIOrWQwXv4GdySuH
bwof3U5p/cq2NDtxv8KGisjK04l++s+Ea8AS6T6O8+08nBFGgfNW331eWtU91JoQ
e6Q4DWipiNzkIvISk48V8CT9eRB2KD7NsigQprePRN3gDZREh+01gwbVUX2gbtHx
1RGxEjO6H0kUuaoXF5E6+WGwgn8MA47qUy1WXC5QDFpc5LyaoVaMFv8bcoWSNXAS
Oes+ZDWDXWq6F+9Kt0zWmO651cVquLTjmgt48fgL6m8rU13ikjH7dFnimrwRxfOD
p+z97N7TvWfgE1HOmYDfsbaHjPFZKg==
-----END CERTIFICATE-----
52 changes: 52 additions & 0 deletions testdata/x509/spiffe_key.pem
View file
Open in desktop
@@ -0,0 +1,52 @@
-----BEGIN PRIVATE KEY-----
MIIJQwIBADANBgkqhkiG9w0BAQEFAASCCS0wggkpAgEAAoICAQCV84YR/EV55qfF
ynHhQvWEZW5hUI9q0DeD5kG5CarrkOj11rZuQIBZ7X23CJbeoVbrvbYLghsPYJzx
S/n3Qlwwzb5k+L0Qt+HrBD836HcSK5k1oh0jGGMaGownap+XCZH9g52s/8iiwfI0
2CmNTbwsNp7wtSEFgNOd2OlzhT6wBLF2Q6uxfBmsDpiChxe2Fs1lyan9RH8fYEf7
sxwPE+SgBfEs7dSG5ZwFfdF+pd1T3IfrVjIxechKO1MO7HTSxbOTj6eHf1NeErDT
GPA7VrnDCupRgcDGyAhFd54r62R8TbTjn5MwzMxElO45Ck/Ej7Qw/GWeaBHj/dMa
6mhER55PvnKuyj+k9t0Rf0HDZyONtY5/OLqI/xVr27Y1o9v5FysNgjWPkZMRpvuC
zkeC2RuE6k2TfBDRLiCyYu/Zzw+ZtUyTAKtWtefLdQBjrYpnhrDPpmrnTWomX/e9
pylEWfkyxCswiPnDw7ypI7uFSTkz0+bUaROmAtlPvR+3SjaQDWigwz3eJsdIaeg5
AY9q//rWaal6l2iR0Ou9L6A9lLxh5iN/ch+OGk4QPK6pFbOy3IqYfmQ+IpAXG0da
9RT2EN76cNa3bldEjRRON8oQ3HZmhOQJqVxhQciUz84sTjAqH8WvqqbdG9HKUoZ1
9T5Z9vNldjlQn33Mi5gBxdugqdnmCQIDAQABAoICADWoJXJsHgRHyAMbtPJRPn94
uC20YQ1somDdVOk8j1+pw+KsSS1cgVEsjU6gkTPq8ap7gRfPH5W6EY66jCCxK0H/
bUC+TREda4boRyLfWTQ0S6eIcfqr8FJX64zzN1YZg5b+sL5F7Opokh3ct8mrZkk/
5lHlzoIknhSemLLQnCTqGQJjpp1k9d6+fk4+vvpWYHsq1VweVYrJrhhf+AthJ+8n
ESztkZ4PrWu9oOg7u94VTMGmX2Ga3VPKtKbjb844FlEYF2+B3TgNYh63jsb8+o3T
axNtZaj7zRHmgr/ehF+CgtbstAPDVNi5niDlErQYY/cfadFsFfLKUe8Qr+y23+vG
1WuVSUmrUcgO/IYMIz2gEOrBOutc9cdKOlCnwrXu3WjSGO6zhcbXCw7WZrSR/Uj5
1Tatt5QJ5Z3i4vOc6Jj1XKL/9Xa+FEryfVh/HKlQTlHnIuuGXMBpIzyYQ6kY8+cH
n75FVMo4lB97c48hweupQY6SUQwvWXqXQOAxLJ/eq2k1QpUWJ4GV5kRr3/eQ/AZ1
y4Kk2ZxM8IWksFdVnomNr65GIk219D1uwDtJQeBrwqrYseGq/2mB2h4llTbwjSez
GkOPO74tLPh3wkG8wDzbc94nfouxCL6ee9W4XeDGzYXgndSKAPOWWUyFnsIxisVu
BB2HUkJZotG2Otrgnj/xAoIBAQDF7NjT9JkN+JhrmH4jG+16lI6RDErf/VgheSE8
G/ayAg1RGY2FsuyTi2bAM3xprXqaZHDSikil8t9G7JJZoKVzesCZR+OJU+9fGZR6
TCS6mCdv6OEG18GJP9dzDLaqYybJ9VgnSnXT7mRlcCyU8uZ9/2FR/vnLEatbOw99
2tles0LGdkKT/YIYdxIENIpmaZhzAOWPDLDwDTO8aIXy40DiGzjLWJf/0LgCV7Ub
2C8aPS4WWXCOdAeYvjcEKeCp9+YSgZaYNT2P8Ns40VcL/yysMtFzvnTOcA9riyzN
5pu+ppv/KYGt/ENa4zQMCgKFtTUxicr2M9VYqNI/CgJmJPKVAoIBAQDB8x13soMv
tohfiNwGTjEkVzu/RkTCix8+hERF1C+oupL3ykpJOGvvpOyhFUqwFAYyBQKflrxj
9lQBKQiPYR0VtgDFJ7UzjYGO8zt8U56gTcYeatpNKY4zvZyGAOBhtWuvuvi8pnpc
xO8yQjE0jWrwWL3bmf/5lP2jO8j+k1qZfrA2ksTGUWRGEZkFRqqTQxvRrJVr9QiY
2xpRq/n7fq9UhCfNxm2aLdYgZ/BCVFzNahCEWfFdH3jOcP5N/5Fkiy7IhwfdglO+
JjydEMqBYHg3ET6MQ3JYM/Gt/GkX0myV9BHd/fYYF2xGBMDIMLVdFKyCivUMGGt1
pHhcNLzebFylAoIBAQDA4Cr4eh8Az2XxTDx3iEqnLsezr8/zcVYF4J2zjuib1YYW
pxkT1iXXLnymBkZSUVztwb10Xo+nMAPHgNipgPRakZ/If5bLh8D34tyfRT5xm76q
vr0zRuPyFQWmtxf2+QKewnjyaQxjx6eMdoDrcb2NwWWcWyYfbwuWrvpMwg0bzQLg
lfQRdXTm1Hn5IR5R6MtIHvKVsV9nvuXQz+bgp/bdoHt7Jc2R3FrE5aW3Cbf1EPOt
keEu4QFaJttEMm8eE1bgZ+pST2e7spJfTxlNtpBZCni0G0CGwAs22PyDdhwF8SSJ
xm/6FZ+pnUlmBgcpN0osCUSBIkfgyzt/dQibc5v1AoIBAF5ZLweQfoLSb+rRf/9N
QFimWvlEbKSa2vslirTRcNHK2T3TWWnfGZq9hyMhYXDgfNcOWuVZhZG3PcxGstRU
8LokDKHcHCjU+KaaqmBjqTHgQ7V+U23f/j4rSh5iBMVjZNxavy++aJ4Caz3ut1MS
TGhZMxrGAqDeGriyl6dH9XXgDEawBStYYsg3PVI0uzviFIFeTF31GFaLl3UNjREL
4qzhkR9oHN841wZyqY0Kzw5aP2iy/FhJvBHpI7y7y3W2w25nSas3ABfrL+dUSL7B
OBnJuLyw/snrkvEJbfJZudsEnUB5j6LOmixBmaqJD2EVcooaoPReWMAk3ywzt4EY
A8UCggEBAJbArCsQ0Q+pFOEce6JlgtYAdcBiu8n5zFYLD2/qZdM2ir/09uj9XDpC
WbE5YTumgzkt2VLK/wf+HATMAyXObtaAn5AdoA6OJ1AvbNGOwA3F2LOlbJGO2XOW
TpQlgbDvBBktaKk9PSszDj92W2tdFQPefDP/uBzonymev7BWCERZ/vU2L3HpXQjp
bxzRyVNWwwg3VBYvbCIz8v2yeviAsiEkOCPRU6+cIr7/VUr0mymzDgfzKaQNoOap
LqOpnInw0pUA+BhsVr4n/fBXZLbSd7ZG5WU48HUaSLefEI4NuiZLT2K1tZreyBqZ
Xgln2zbN6APAb+dGDdv27dz4YlasU4s=
-----END PRIVATE KEY-----