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

Add E2E tests for OIDC token validation #323

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alegacy
Copy link

@alegacy alegacy commented Nov 13, 2024

This adds end-to-end tests and supporting material to execute tests that validate parts of the OIDC functionality.

Current tests include:

  1. Using a valid token
  2. Using an expired token
  3. Using a token signed by an unknown signer
  4. Using a token for an unknown user
  5. Using a token matching a group binding
  6. Using a token for an audience not matching the configured client-id

@alegacy alegacy force-pushed the feature/add-e2e-tests-for-oidc branch from 042bd58 to c04635c Compare November 13, 2024 20:10
Copy link
Collaborator

@stlaz stlaz left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request.

Please don't add private keys to this repository - ideally we would remove them from the manifests, too. Our test suite is in golang so please move your bash tests into golang.

Makefile Outdated Show resolved Hide resolved
scripts/generate-certificates.sh Outdated Show resolved Hide resolved
test/data/oauth-token-signer.key Outdated Show resolved Hide resolved
@alegacy
Copy link
Author

alegacy commented Nov 15, 2024

Please don't add private keys to this repository - ideally we would remove them from the manifests, too. Our test suite is in golang so please move your bash tests into golang.

@stlaz My apologies. Was just following what already existed in this repo since the private key for client testing is already stored. I'll do my best to refactor.

@stlaz
Copy link
Collaborator

stlaz commented Nov 18, 2024

No need to apologize 🙂 I don't think there were private keys before in the repo, which is a little bit of a difference. I believe Red Hat infosec detects that at least in their repos, not sure if by any chance for all of their employees. so you may still be getting an email from them 😁

I believe that generating cryptomaterial as a golang fixture will be a great improvement to the test suite, actually, and would be very welcome 👍

@alegacy
Copy link
Author

alegacy commented Nov 18, 2024

No need to apologize 🙂 I don't think there were private keys before in the repo, which is a little bit of a difference. I believe Red Hat infosec detects that at least in their repos, not sure if by any chance for all of their employees. so you may still be getting an email from them 😁
😄
... what I meant was that the private key is currently in the clientcertificates/certificate.yaml

I reworked my changes on the weekend and now have fixtures for the certs and tokens. Still need to do a bit of testing before I update the PR with the new code.

Thanks for the quick review.

@alegacy alegacy force-pushed the feature/add-e2e-tests-for-oidc branch from c04635c to f6d634d Compare November 19, 2024 15:29
@alegacy
Copy link
Author

alegacy commented Nov 19, 2024

@stlaz Pushed new changes with an attempt at refactoring to create certs/tokens dynamically. Please take another look when you have a chance.

@alegacy alegacy requested a review from stlaz November 22, 2024 13:59
Copy link
Collaborator

@stlaz stlaz left a comment

Choose a reason for hiding this comment

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

Thank you, this must have been a lot of work.

I think we'll need to change the mockserver though, the current one is unmaintained.

go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
test/e2e/main_test.go Outdated Show resolved Hide resolved
github.com/moby/term v0.5.0 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f // indirect
Copy link
Collaborator

Choose a reason for hiding this comment

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

that's a version from 10 years ago 😱

test/e2e/oidc.go Outdated Show resolved Hide resolved
test/kubetest/tls.go Outdated Show resolved Hide resolved
test/kubetest/tls.go Outdated Show resolved Hide resolved
test/kubetest/tls.go Outdated Show resolved Hide resolved
test/kubetest/tls.go Show resolved Hide resolved
test/kubetest/tls.go Outdated Show resolved Hide resolved
@stlaz stlaz self-assigned this Nov 25, 2024
@alegacy
Copy link
Author

alegacy commented Nov 25, 2024

I think we'll need to change the mockserver though, the current one is unmaintained.

Is there an alternative that you would recommend?

@alegacy
Copy link
Author

alegacy commented Nov 25, 2024

@stlaz You've added comments to files that I've copied in directly from the sig-auth-acceptance branch from a commit that @ibihim authored. Do you want those changed considering that it will create merge headaches between those two branches?

@stlaz
Copy link
Collaborator

stlaz commented Nov 26, 2024

Is there an alternative that you would recommend?

An ideal alternative would be something we can actually run in a container but that might be hard. Basically you'd have an HTTP server and you'd instruct it with how it should respond to certain queries.
I remember looking for something like that before, and for OIDC mock servers, but there's probably nothing like that anywhere.

kubernetes/kubernetes is using https://github.com/uber-go/mock for mocking (it's actually using an older version that was originally in the golang org). Not sure whether we could actually use it here, too.

@alegacy
Copy link
Author

alegacy commented Nov 26, 2024

Is there an alternative that you would recommend?

An ideal alternative would be something we can actually run in a container but that might be hard. Basically you'd have an HTTP server and you'd instruct it with how it should respond to certain queries. I remember looking for something like that before, and for OIDC mock servers, but there's probably nothing like that anywhere.

mock-server is the most popular out there as far as I can tell even though the maintainer seems to have disappeared. Given its popularity i would hope someone would pick it up, but if you don't like depending on that then may https://wiremock.org/ might work, i'll take a look at what its capabilities are.

@alegacy
Copy link
Author

alegacy commented Nov 27, 2024

An ideal alternative would be something we can actually run in a container but that might be hard. Basically you'd have an HTTP server and you'd instruct it with how it should respond to certain queries. I remember looking for something like that before, and for OIDC mock servers, but there's probably nothing like that anywhere.

That's exactly what mock-server does. It's also what wiremock does -- i'm testing this one now to see if it'll do what we need.

ibihim and others added 4 commits December 14, 2024 19:52
This partially cherry-picks commit 204148c from the sig-auth-acceptance
branch so that the certificate utilities can be used.

Signed-off-by: Allain Legacy <[email protected]>
Unify cert template generation. SKID and AKID should be properly
computed by Golang, no need to add them explicitly - unless we need
explicit SKID but that's not the case in our tests.
The previous design was mixing CA and leaf certificates, making it
easy to confuse them at place of use. Have all the leaf cryptomaterial
in a secret, and the trust in the CM to avoid those issues.
This adds end-to-end tests and supporting material to execute tests that
validate parts of the OIDC functionality.

Signed-off-by: Allain Legacy <[email protected]>
@alegacy alegacy force-pushed the feature/add-e2e-tests-for-oidc branch from f6d634d to 634fe0a Compare December 16, 2024 12:42
@alegacy
Copy link
Author

alegacy commented Dec 16, 2024

@stlaz I've updated with changes to use wiremock instead of mock-server. Please have another look.

@alegacy alegacy requested a review from stlaz December 16, 2024 12:48
@alegacy
Copy link
Author

alegacy commented Jan 8, 2025

@stlaz Any chance you could re-review this so we can move this forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants