feat: simplify token audiences config #947
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package cluster | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "reflect" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| authentication "k8s.io/api/authentication/v1" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| var ( | ||
| // Default value of audiences for DSCI.SM.auth. | ||
| defaultAudiences = []string{"https://kubernetes.default.svc"} | ||
| ) | ||
|
|
||
| func isDefaultAudiences(specAudiences *[]string) bool { | ||
| return specAudiences == nil || reflect.DeepEqual(*specAudiences, defaultAudiences) | ||
| } | ||
|
|
||
| type TokenSupplier func() (string, error) | ||
|
|
||
| // GetEffectiveClusterAudiences returns the audiences defined for the cluster | ||
| // falling back to the default audiences in case of errors. | ||
| func GetEffectiveClusterAudiences(cli client.Client, log logr.Logger, specAudiences *[]string, getTokenFunc TokenSupplier) []string { | ||
| if isDefaultAudiences(specAudiences) { | ||
| return fetchClusterAudiences(cli, log, getTokenFunc) | ||
| } | ||
| return *specAudiences | ||
| } | ||
|
|
||
| func fetchClusterAudiences(cli client.Client, log logr.Logger, getTokenFunc TokenSupplier) []string { | ||
| token, err := getTokenFunc() | ||
| if err != nil { | ||
| log.Error(err, "Error getting token, using default audiences") | ||
| return defaultAudiences | ||
| } | ||
|
|
||
| tokenReview := &authentication.TokenReview{ | ||
| Spec: authentication.TokenReviewSpec{ | ||
| Token: token, | ||
| }, | ||
| } | ||
|
|
||
| if err = cli.Create(context.Background(), tokenReview, &client.CreateOptions{}); err != nil { | ||
| log.Error(err, "Error creating TokenReview, using default audiences") | ||
| return defaultAudiences | ||
| } | ||
|
|
||
| if tokenReview.Status.Error != "" || !tokenReview.Status.Authenticated { | ||
| log.Error(fmt.Errorf(tokenReview.Status.Error), "Error with token review authentication status, using default audiences") | ||
| return defaultAudiences | ||
| } | ||
|
|
||
| return tokenReview.Status.Audiences | ||
|
There was a problem hiding this comment. Reading these docs: https://kubernetes.io/docs/reference/access-authn-authz/authentication/. There is a note that the audiences field is optional and that, if not present, it may still mean the token is valid for the cluster. So, I'm not sure if this is safe. There was a problem hiding this comment. Underlying AuthConfig will be defaulted if not configured specifically. Current implementation defaults it to There was a problem hiding this comment. In the docs that I linked, there is this text over
IIRC, this default is on odh-model-controller side. This is OK. My concern is that my understanding of the kubernetes docs is that the cluster may choose to omit the The writing is somewhat confusing. So, I may be understanding things badly. |
||
| } | ||
|
There was a problem hiding this comment. I think we should stick to the correct terminology. We are providing canned responses, so this is stubbing, not mocking. A bit more about it in this great blog post https://martinfowler.com/articles/mocksArentStubs.html |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| package cluster_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| authentication "k8s.io/api/authentication/v1" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
|
|
||
| "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| var defaultAudiences = []string{"https://kubernetes.default.svc"} | ||
|
|
||
| var _ = Describe("Handling Token Audiences", func() { | ||
| var fakeClient client.Client | ||
| var logger logr.Logger | ||
|
|
||
| BeforeEach(func() { | ||
| fakeClient = fake.NewClientBuilder().WithScheme(testScheme).Build() | ||
| logger = logr.Logger{} | ||
| }) | ||
|
|
||
| Context("Determining the effective cluster audiences", func() { | ||
| When("non-default audiences are provided", func() { | ||
| It("should return the provided audiences", func() { | ||
| specAudiences := []string{"https://example.com"} | ||
| Expect(cluster.GetEffectiveClusterAudiences(fakeClient, logger, &specAudiences, cluster.GetSAToken)).To(Equal(specAudiences)) | ||
| }) | ||
| }) | ||
|
|
||
| When("non-default audiences are fetched successfully", func() { | ||
| It("should return the fetched audiences", func() { | ||
| mockAudiences := []string{"https://mock.audience.com"} | ||
| mockGetSAToken := func() (string, error) { | ||
| return "mock_token", nil | ||
| } | ||
|
|
||
| baseClient := fake.NewClientBuilder().WithScheme(testScheme).Build() | ||
| fakeClient := &mockAudiencesClient{Client: baseClient, mockAudiences: mockAudiences} | ||
|
|
||
| Expect(cluster.GetEffectiveClusterAudiences(fakeClient, logger, nil, mockGetSAToken)).To(Equal(mockAudiences)) | ||
| }) | ||
| }) | ||
|
|
||
| When("an error occurs while fetching the audiences", func() { | ||
| It("should return the default audiences", func() { | ||
| mockFailGettingToken := func() (string, error) { | ||
| return "", fmt.Errorf("we failed getting token") | ||
| } | ||
|
|
||
| Expect(cluster.GetEffectiveClusterAudiences(fakeClient, logger, nil, mockFailGettingToken)).To(Equal(defaultAudiences)) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| type mockAudiencesClient struct { | ||
| client.Client | ||
| mockAudiences []string | ||
| } | ||
|
|
||
| func (mac *mockAudiencesClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { | ||
| if tokenReview, isTokenReview := obj.(*authentication.TokenReview); isTokenReview { | ||
| tokenReview.Status.Audiences = mac.mockAudiences | ||
| tokenReview.Status.Authenticated = true | ||
| return nil | ||
| } | ||
| return mac.Client.Create(ctx, obj, opts...) | ||
| } | ||
bartoszmajsak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This seems to use the ServiceAccount mounted by the cluster, as a pivot to find the audientes.
I wonder if it isn't enough to parse the ServiceAccount token?
Just by parsing the one from CRC, I see the audiences claim in the payload:
I'm not sure how it would be for ROSA.
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.
Correct, originally we were using opaque token for the bash "workaround" and this solution is based on this, so perhaps, using SA (jwt) token aud is simpler and more efficient. We could replace this impl and use https://github.com/golang-jwt/jwt instead to fetch standard claims without verification (so passing
nilas the key func ). @israel-hdez @aslakknutsen do you think it's better to rely on the fact that SA token is JWT?I just checked in ROSA and we can also parse JWT instead. For opaque (openshift oauth) tokens it's the same audience. We can get it by using the exact call to TokenReview API.
This behavior is only invoked when you do not specify anything in DSCI.ServiceMesh.Auth part. If you later do, it will update underlying config map which is now used by
odh-model-controllerThere 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.
AFAIK, core Kubernetes only supports JWT (read 1st sentence of this). So, I'd expect any SA token to be JWT. I understand we have control about the mounted/projected token so, as long as the setup does not change, we should be safe to assume it is a JWT.
About the opaque tokens, my understanding is that in core Kubernetes these are only supported when a 3rd party issues the token and the API Server would validate the token via a Webhook to the 3rd party (i.e. Kubernetes does not validate by its own). OpenShift's OAuth server may fall in this category.