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

DoCertExchange function does not use certificates in request to Keycloak #1008

Closed
0xArch3r opened this issue Jun 18, 2024 · 7 comments
Closed
Labels
bug Something isn't working

Comments

@0xArch3r
Copy link

I am trying to utilize the sdk to exchange x509 certificates for AccessTokens to be used against the platform. After multiple attempts all resulting in {"error_description":"X509 client certificate is missing.","error":"invalid_request"} I decided to try and authenticate directly with keycloak first. I was successful and retrieved a jwt from keycloak.

This made me dive into the sdk code and I noticed that the DoCertExchange workflow never adds any of the headers or certificates needed to authenticate with the IDP and get the access token.

@ttschampel
Copy link
Member

Can you provide more details on your Keycloak setup? What type of x509 Auth flow do you have setup for Keycloak? The standard(default) Keycloak x509 Cert Lookup Provider does not required any custom headers

@ttschampel
Copy link
Member

ttschampel commented Jun 18, 2024

See this Unit test for an example of setting up TLS config passed into the SDK

@0xArch3r
Copy link
Author

0xArch3r commented Jun 18, 2024

Not sure if they HAVE to be headers for this type of authentication, however, this is what worked for me when trying to authenticate directly with keycloak

curl -XPOST https://<keycloak ip>:8443/auth/realms/opentdf/protocol/openid-connect/token --cacert \ ca.crt --data "grant_type=password&scope=openid profile&client_id=opentdf-sdk&client_secret=secret&username=&password=&audience=opentdf-sdk" \ -E user_certificate.crt --key user_private_key.key --header "ssl-client-cert: $(urlencode "$(cat user_certificate.crt)")" \ --header "ssl-client-issuer-dn: CN=Root CA" \ --header "ssl-client-subject-dn: emailAddress=user@internal.domain,CN=user,OU=Foo,O=Bar,L=Hello,ST=Test,C=US" \ --header "ssl-client-verify: SUCCESS" -v

I got this from a keycloak discussion board.

With that being said though, the Cert exchange code in the SDK does nothing with the Certificates tht are passed with the sdk.WithTLSCredentials(). It does add the audience from the CertExchangeInfo struct, but it never does anything with the underlying certificates.

Ive seen that unit test, and unfortuntely a lot of those functions are internal so when importing the sdk we don't have access to them. The only way to instantiate the cert exchange is through the sdk.New() passing it sdk.WithTLSCredentials()

func DoCertExchange(ctx context.Context, client *http.Client, tokenEndpoint string, exchangeInfo CertExchangeInfo, clientCredentials ClientCredentials, key jwk.Key) (*Token, error) {
	req, err := getCertExchangeRequest(ctx, tokenEndpoint, clientCredentials, exchangeInfo, key)
	if err != nil {
		return nil, err
	}
	resp, err := client.Do(req)
	if err != nil {
		return nil, fmt.Errorf("error making request to IdP for certificate exchange: %w", err)
	}
	defer resp.Body.Close()

	return processResponse(resp)
}

func getCertExchangeRequest(ctx context.Context, tokenEndpoint string, clientCredentials ClientCredentials, exchangeInfo CertExchangeInfo, key jwk.Key) (*http.Request, error) {
	data := url.Values{"grant_type": {"password"}, "client_id": {clientCredentials.ClientID}, "username": {""}, "password": {""}}

	for _, a := range exchangeInfo.Audience {
		data.Add("audience", a)
	}

	body := strings.NewReader(data.Encode())
	req, err := http.NewRequestWithContext(ctx, http.MethodPost, tokenEndpoint, body)
	if err != nil {
		return nil, err
	}

	dpop, err := getDPoPAssertion(key, http.MethodPost, tokenEndpoint, "")
	if err != nil {
		return nil, err
	}
	req.Header.Set("accept", "application/json")
	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
	req.Header.Set("dpop", dpop)
	if err = setClientAuth(clientCredentials, &data, req, tokenEndpoint); err != nil {
		return nil, err
	}

	return req, nil
}

This is the code in those functions that are used. exchangeinfo is the struct that has the TLS Config and the array of audiences... in the getCertExchangeRequest function the only time it accesses that object is when it takes the audience and adds it to the body... but in the NewRequestWithContext we only pass that body with audience (no certs) and token endpoint.

When running this end to end the keycloak server always responds with no x509 certificate.

@0xArch3r
Copy link
Author

When you pass sdk.WithTLSCredentials into the sdk.New() it does go through and run the DoCertExchange internally, but we cannot just stand it up the way you do in the unit test due to Go's internal restriction on packages

@0xArch3r
Copy link
Author

I do notice, that in the Unit Test, the TLS Config passed to the client is the one where the sampleuser certificates are being applied, this may be why there doesnt need to be any additional custom headers. With that being said, when using the sdk.New() the TLS Config that gets passed into the client that is passed into func (c *CertExchangeTokenSource) AccessToken(ctx context.Context, client *http.Client) is not the TLS Config from the WithTLSCredentials, but it is from the config.TLSConfig... when you use WithTLSCredentials it sets the config.certExchange property, not the TLSConfig. This may be where the disconnect is from sdk.New and the unit test

@0xArch3r 0xArch3r changed the title DoCertExchange function does not use certirficates in request to keycloak DoCertExchange function does not use certificates in request to Keycloak Jun 19, 2024
@0xArch3r
Copy link
Author

0xArch3r commented Jun 19, 2024

Just to further test, I changed this

func DoCertExchange(ctx context.Context, client *http.Client, tokenEndpoint string, exchangeInfo CertExchangeInfo, clientCredentials ClientCredentials, key jwk.Key) (*Token, error) {
	req, err := getCertExchangeRequest(ctx, tokenEndpoint, clientCredentials, exchangeInfo, key)
	if err != nil {
		return nil, err
	}
	resp, err := client.Do(req)
	if err != nil {
		return nil, fmt.Errorf("error making request to IdP for certificate exchange: %w", err)
	}
	defer resp.Body.Close()

	return processResponse(resp)
}

to this:

func DoCertExchange(ctx context.Context, client *http.Client, tokenEndpoint string, exchangeInfo CertExchangeInfo, clientCredentials ClientCredentials, key jwk.Key) (*Token, error) {
	req, err := getCertExchangeRequest(ctx, tokenEndpoint, clientCredentials, exchangeInfo, key)
	if err != nil {
		return nil, err
	}
        client.Transport = &http.Transport{
		TLSClientConfig: exchangeInfo.TLSConfig,
	}
	resp, err := client.Do(req)
	if err != nil {
		return nil, fmt.Errorf("error making request to IdP for certificate exchange: %w", err)
	}
	defer resp.Body.Close()

	return processResponse(resp)
}

And this caused the authentication to be successful, by adding the TLS config that is set in exchangeInfo versus the on config in the TLS Config for the sdk.

@ttschampel
Copy link
Member

Fixed by #1043

github-merge-queue bot pushed a commit that referenced this issue Jul 2, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.2.9](sdk/v0.2.8...sdk/v0.2.9)
(2024-07-02)


### Features

* **sdk:** support unsafe policy service in SDK
([#1076](#1076))
([ca88554](ca88554))


### Bug Fixes

* **core:** Autobump sdk
([#1070](#1070))
([4ca372c](4ca372c))
* Issue [#1008](#1008) : Use
exchange info's TLS Configuration for cert based auth
([#1043](#1043))
([93d8f70](93d8f70))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
jakedoublev pushed a commit that referenced this issue Jul 7, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.2.9](sdk/v0.2.8...sdk/v0.2.9)
(2024-07-02)


### Features

* **sdk:** support unsafe policy service in SDK
([#1076](#1076))
([ca88554](ca88554))


### Bug Fixes

* **core:** Autobump sdk
([#1070](#1070))
([4ca372c](4ca372c))
* Issue [#1008](#1008) : Use
exchange info's TLS Configuration for cert based auth
([#1043](#1043))
([93d8f70](93d8f70))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants