-
Notifications
You must be signed in to change notification settings - Fork 542
Feat: Support client JWT auth method for kafka oidc #4057
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
Conversation
Signed-off-by: Albert Callarisa <[email protected]>
c23e275
to
9cd61ec
Compare
2e0fe18
to
6cb1166
Compare
Signed-off-by: Albert Callarisa <[email protected]>
6cb1166
to
742b291
Compare
common/component/kafka/metadata.go
Outdated
if m.OidcClientAuthMethod == "client_secret" && m.OidcClientSecret == "" { | ||
return nil, errors.New("kafka error: missing OIDC Client Secret for authType 'oidc' (client_secret)") | ||
} | ||
if m.OidcClientAuthMethod == "client_jwt" && m.OidcClientAssertionCert == "" { | ||
return nil, errors.New("kafka error: missing OIDC Client Assertion Cert for authType 'oidc' (client_jwt)") | ||
} | ||
if m.OidcClientAuthMethod == "client_jwt" && m.OidcClientAssertionKey == "" { | ||
return nil, errors.New("kafka error: missing OIDC Client Assertion Key for authType 'oidc' (client_jwt)") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can we use pointers to signal presence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used empty strings for consistence with the rest of the metadata fields. I think it'd be better to change it all in a different PR, wdyt?
block, _ := pem.Decode([]byte(ts.ClientAssertionKey)) | ||
if block == nil { | ||
return nil, errors.New("invalid PEM private key for client assertion") | ||
} | ||
pk, err := x509.ParsePKCS8PrivateKey(block.Bytes) | ||
if err != nil { | ||
// try PKCS1 | ||
if rsaKey, err2 := x509.ParsePKCS1PrivateKey(block.Bytes); err2 == nil { | ||
pk = rsaKey | ||
} else { | ||
return nil, fmt.Errorf("unable to parse private key: %w", err) | ||
} | ||
} | ||
rsaKey, ok := pk.(*rsa.PrivateKey) | ||
if !ok { | ||
return nil, errors.New("client_jwt requires RSA private key") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use dapr/kit/crypto/pem
.
Subject(ts.ClientID). | ||
Audience([]string{audClaim}). | ||
IssuedAt(now). | ||
Expiration(now.Add(1 * time.Minute)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 1 minute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's arbitrary, it's just a short lived token used to communicate with the IdP. It could be even shorter in practice. Do you think we should make it shorter, like 5 seconds or so maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls ensure the docs reflect this as well when the time comes 🙏
urlValues.Set("scope", strings.Join(ts.Scopes, " ")) | ||
} | ||
|
||
timeoutCtx, cancel := ctx.WithTimeout(ctx.TODO(), tokenRequestTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why TODO
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was TODO
before too, I don't know the reason. Other places use the background context 🤷
The sarama.AccessTokenProvider
(docs) doesn't take a context, wiring it up with a provided context would change a bit too much. Should we use background instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should take a context which is passed through
defer cancel() | ||
ts.configureClient() | ||
req, err := http.NewRequestWithContext(timeoutCtx, http.MethodPost, ts.TokenEndpoint.TokenURL, strings.NewReader(urlValues.Encode())) | ||
if err != nil { | ||
return nil, err | ||
} | ||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | ||
resp, err := ts.httpClient.Do(req) | ||
if err != nil { | ||
return nil, err | ||
} | ||
defer resp.Body.Close() | ||
if resp.StatusCode < 200 || resp.StatusCode >= 300 { | ||
return nil, fmt.Errorf("token endpoint returned %d", resp.StatusCode) | ||
} | ||
var tr tokenResponse | ||
if err := json.NewDecoder(resp.Body).Decode(&tr); err != nil { | ||
return nil, err | ||
} | ||
if tr.AccessToken == "" { | ||
return nil, errors.New("no access_token in response") | ||
} | ||
ts.CachedToken = oauth2.Token{AccessToken: tr.AccessToken, Expiry: time.Now().Add(time.Duration(tr.ExpiresIn) * time.Second)} | ||
return ts.asSaramaToken(), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there not something that does all this for us in a library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any unfortunately
-----BEGIN CERTIFICATE----- | ||
MIIEJTCCAg2gAwIBAgIUS0RIkoWlMVd8UfE/I7IEzKD4kmwwDQYJKoZIhvcNAQEL | ||
BQAwGDEWMBQGA1UEAwwNbG9jYWwtb2lkYy1jYTAeFw0yNTEwMDMxMDI3NDlaFw0y | ||
ODAxMDYxMDI3NDlaMBsxGTAXBgNVBAMMEGRhcHItb2lkYy1jbGllbnQwggEiMA0G | ||
CSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDQIqGyGGlQRqXNDlSuHRDGmnJ4dQDb | ||
jK/KxeC+wXQiDCSfHgP7NOjZOOCiQYKAUs9MRg+tKepxFRORZLAbRlU9kuU4Z7bg | ||
E6wDMztCqTASUZtwZeN36iJDcQ++xnmAi8j0bvs7w00wrgPMXPnT09Sf9sQyEkR2 | ||
t26OLcQnZHwrUWHIbZeXqvRF7uYG00GKjFmqI+FjrmDx0hoENew+Jzpk7S/7m5t2 | ||
h/weVAr5REtGTIQxbX8nNmoO2JFbyILBcfP9M9R9zxiTEFVgP5sl2QfLFerPpULM | ||
1tAiCO+oPk8CAFiD9TKe+HR/Us4uIxgRmxjgBnzmFSJjDflvK2DslpzPAgMBAAGj | ||
ZDBiMAsGA1UdDwQEAwIFoDATBgNVHSUEDDAKBggrBgEFBQcDAjAdBgNVHQ4EFgQU | ||
oCgedGjEdPHOBRq6OwKKGUmpUXIwHwYDVR0jBBgwFoAUImUtm30c0E0Z1Xsep9o/ | ||
nXsqyGYwDQYJKoZIhvcNAQELBQADggIBAKafBCz1EUny2pKOI8xTHVkoZjhTvtm2 | ||
Ly2DicAEwtESXlBta64bOug4mTq8RLdKxLa8xSBSZxzygTfb+q8IcuuYL0/DAttZ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use hard coded certificates and keys in tests. They get copied and used in the real world, and they also expire.
Always generate them in the test, and use secret refs
Signed-off-by: Albert Callarisa <[email protected]>
Signed-off-by: Albert Callarisa <[email protected]>
404144d
to
c3489f7
Compare
Signed-off-by: Albert Callarisa <[email protected]>
c3489f7
to
8481041
Compare
tests/certification/pubsub/kafka/components/auth_oidc_certs/kafka.yaml
Outdated
Show resolved
Hide resolved
tests/certification/pubsub/kafka/components/auth_oidc_certs/kafka.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Albert Callarisa <[email protected]>
if m.OidcClientAssertionKey == "" { | ||
return nil, errors.New("kafka error: missing OIDC Client Assertion Key for authType 'oidc_private_key_jwt'") | ||
} | ||
if m.OidcScopes != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls add this field to the auth profile in the metadata.yaml as I don't see it with the openid string default
k.logger.Warn("Warning: no OIDC scopes specified, using default 'openid' scope only. This is a security risk for token reuse.") | ||
m.internalOidcScopes = []string{"openid"} | ||
} | ||
if m.OidcExtensions != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is also missing in the metadata.yaml file. So this is an optional field?
Signed-off-by: Albert Callarisa <[email protected]>
Ref: dapr/components-contrib#4057 Signed-off-by: Albert Callarisa <[email protected]>
Ref: #4003
At the moment the
oidc
auth type only supports authentication usingclientID
andclientSecret
.This PR adds a new auth type
oidc_private_key_jwt
that uses certificates instead. The component will use those certificates to build the client assertion and call the IdP to get a token.This new auth type uses the following new fields in the kafka components:
oidcClientAssertionCert
oidcClientAssertionKey
oidcResource
oidcAudience
In this PR I also added tests for both oidc authentication mechanisms against a local kafka, using keycloak as an IdP.