Implements OIDC distributed claims. #63213

filmil · 2018-04-26T19:51:58Z

Next step to enable this feature is to enable claim caching.

A distributed claim allows the OIDC provider to delegate a claim to a
separate URL. Distributed claims are of the form as seen below, and are
defined in the OIDC Connect Core 1.0, section 5.6.2.

See: https://openid.net/specs/openid-connect-core-1_0.html#AggregatedDistributedClaims

Example claim:

{
  ... (other normal claims)...
  "_claim_names": {
    "groups": "src1"
  },
  "_claim_sources": {
    "src1": {
      "endpoint": "https://www.example.com",
      "access_token": "f005ba11"
    },
  },
}

Example response to a followup request to https://www.example.com is a
JWT-encoded claim token:

{
  "iss": "https://www.example.com",
  "aud": "my-client",
  "groups": ["team1", "team2"],
  "exp": 9876543210
}

Apart from the indirection, the distributed claim behaves exactly
the same as a standard claim. For Kubernetes, this means that the
token must be verified using the same approach as for the original OIDC
token. This requires the presence of "iss", "aud" and "exp" claims in
addition to "groups".

All existing OIDC options (e.g. groups prefix) apply.

Any claim can be made distributed, even though the "groups" claim is
the primary use case.

Allows groups to be a single string due to
#33290, even though
OIDC defines "groups" claim to be an array of strings. So, this will
be parsed correctly:

{
  "iss": "https://www.example.com",
  "aud": "my-client",
  "groups": "team1",
  "exp": 9876543210
}

Expects that distributed claims endpoints return JWT, per OIDC specs.

In case both a standard and a distributed claim with the same name
exist, standard claim wins. The specs seem undecided about the correct
approach here.

Distributed claims are resolved serially. This could be parallelized
for performance if needed.

Aggregated claims are silently skipped. Support could be added if
needed.

What this PR does / why we need it: Makes it possible to retrieve many group memberships by offloading to a dedicated backend for groups resolution.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #62920

Special notes for your reviewer:
There are a few TODOs that seem better handled in separate commits.

Release note:

Lays groundwork for OIDC distributed claims handling in the apiserver authentication token checker.

A distributed claim allows the OIDC provider to delegate a claim to a
separate URL.  Distributed claims are of the form as seen below, and are
defined in the OIDC Connect Core 1.0, section 5.6.2.

For details, see: 
http://openid.net/specs/openid-connect-core-1_0.html#AggregatedDistributedClaims

filmil · 2018-04-26T19:52:32Z

This is to handle: #62920

mikedanese · 2018-04-26T20:29:39Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	//
+	// Example:
+	// "groups" -> []string{"http://example1.com/foo","http://example2.com/bar"}
+	IssuersPerClaim map[string][]string


why shouldn't the issuer decide rather than the k8s oidc client? what does this solve?

I'm also for not adding new APIs.

Two benefits, IMHO:

Makes an explicit guarantee to the cluster owner that only issuers they explicitly approve of will ever be called to resolve distributed claims. Note that it's not a decision by the oidc client; but a restriction on the acceptable values.

Makes it possible for us to initialize the ID token provider ahead of time.

@ericchiang could you please clarify your comment? What new APIs are being added? Perhaps more importantly, what would you like to see instead?

What new APIs are being added?

New flags.

Makes an explicit guarantee to the cluster owner that only issuers they explicitly approve of will ever be called to resolve distributed claims. Note that it's not a decision by the oidc client; but a restriction on the acceptable values.

Users already put a lot of trust in their primary ID Token issuer. I feel like the benefit is small compared to adding more configuration nobs.

Let me think about this a little.

Thinking this this a little more, the only distributed claims we should allow are the username
and groups claim (edit: or maybe even only allow it for the groups claim).

Could you explain why you think this is the case? (e.g. for example, only nonstandard claims are allowed to be distributed?)

I would not expect a distributed claim to be able to provide the expiry, for example.

Hm, I have a slightly different view, though I'm not an expert in the matter. It seems that a distributed claim response must provide the expiry.

Otherwise, it becomes similar to a long-lived credential, which could be stolen and replayed. With a timestamp, whatever is examining the credential has a way to determine if the credential is fresh or not and therefore if it can believe the claims. This seems like quite an important requirement from a security perspective.

If we're worried about what remote addresses we can consult for this maybe we whitelist issuers?

That would work.

However, when would an issuer provide a distributed claim that an admin wouldn't be comfortable with? If the admin is picking a username or group claim they're surely aware of if that claim can be distributed and where it'd be distributed to.

I think from an auditability perspective it is much stronger to claim "we know exactly what calls our API server is making" than "our API server may be redirected to wherever our IDP says".

But if you think that's not a big concern, I'll be happy to oblige.

I would not expect a distributed claim to be able to provide the expiry, for example.

Hm, I have a slightly different view, though I'm not an expert in the matter. It seems that a distributed claim response must provide the expiry.

I think you're talking about two different things. First, we verify the presented id_token, including its iss, exp, nbf claims. That is the exp claim Eric was saying he wouldn't expect to be able to be distributed.

Once verified, we look for the username and group claims. Those could be distributed, and if they are, according to the spec, the response from the distributed endpoint is another JWT that must be verified (including iss, and potentially its own exp, and nbf claims)

First, we verify the presented id_token, including its iss, exp, nbf claims. That is the exp claim Eric was saying he wouldn't expect to be able to be distributed.

Ah I see. Indeed, not all claims may be named in _claim_names. In this implementation, a normal claim always wins if there is a claim name clash, which means exp will never be resolved through _claim_sources. But, let's make that statement even stronger, then.

To summarize my understanding of what would be acceptable to you:

Only allow groups claim to be distributed (that is, whatever the value has been set in the OIDC configuration to mean groups).

Require admins to specify --oidc-allowed-distributed-claims-issuer to list all acceptable issuers.

Introduce --oidc-disallow-distributed-claims as an escape hatch.

What do you think?

Only allow groups claim to be distributed (that is, whatever the value has been set in the OIDC configuration to mean groups).

+1

Require admins to specify --oidc-allowed-distributed-claims-issuer to list all acceptable issuers.

-1

I still think that admins are actually more tolerant of requests to different sites than you expect. accounts.google.com returns a keys URL hosted under www.googleapis.com, for example.

(edit: this is also a restriction we could add later in a backwards compatible way)

Introduce --oidc-disallow-distributed-claims as an escape hatch.

I'd like to see someone request this behavior first.

OK, I scaled back the implementation to support only groups claim resolution through distributed claims. Also removed all flags and options. Better?

mikedanese · 2018-04-26T20:32:35Z

@kubernetes/sig-auth-pr-reviews

awly

A bunch of stylistic comments but deferring to Eric/Mike on the overall design

awly · 2018-04-26T20:57:00Z

pkg/kubeapiserver/options/authentication.go

@@ -230,6 +242,9 @@ func (s *BuiltInAuthenticationOptions) AddFlags(fs *pflag.FlagSet) {
 			"A key=value pair that describes a required claim in the ID Token. "+
 			"If set, the claim is verified to be present in the ID Token with a matching value. "+
 			"Repeat this flag to specify multiple claims.")
+
+		fs.Var(flag.NewColonSeparatedMultimapStringString(&s.OIDC.IssuersPerClaim), "oidc-distributed-claims-issuers-per-claim", ""+


Remove ""+ at the end

awly · 2018-04-26T20:58:34Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/BUILD

@@ -8,6 +8,7 @@ load(

 go_test(
    name = "go_default_test",
+	size = "small",


Indent with spaces (I think /hack/update-bazel.sh should reformat this)

Hm, on a second thought, let's revert this change altogether.

awly · 2018-04-26T21:01:36Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+}
+
+// initVerifier creates a new ID token verifier for the given configuration and issuer URL.  On success, calls setVerifier with the
+// resulting verifier.  Returns true in case of success, or non-nil error in case of an irrecoverable error.


The last sentence is redundant.

awly · 2018-04-26T21:05:13Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	// Polls indefinitely in an attemt to initialize the distributed claims
+	// verifier, or until context canceled.
+	sync := make(chan struct{})
+	initFn := func() (done bool, err error) {


err is always nil. Should this be func() bool ?

It has to conform to wait.PollUntil API.

awly · 2018-04-26T21:06:09Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+		return true, nil
+
+	}
+	go func() {


Why not simply go wait.PollUntil(time.Second*10, initFn, ctx.Done()) ?

wait.PollUntil waits for 10 seconds before it executes initFn. I'd prefer to not have to wait if possible. The package wait does not seem to have a function that both has a done channel and executes first before pause, as far as I could see.

I see. You could use wait.PollImmediateInfiite and move return ctx.Err() from initFn to catch cancelled context

awly · 2018-04-26T21:25:27Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+		return fmt.Errorf("oidc: no claim sources")
+	}
+
+	var sources claims


can this be map[string]endpoint so you don't have to unmarshal endpoints in the loop below?

Lo and behold, it can!

Done. :)

awly · 2018-04-26T21:29:33Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	defer cancel()
+
+	// TODO: Allow passing request body with configurable information.
+	req, _ := http.NewRequest("GET", url, nil)


handle the error

awly · 2018-04-26T21:30:26Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	if accessToken != "" {
+		req.Header.Set("Authorization", fmt.Sprintf("Bearer %v", accessToken))
+	}
+	req.WithContext(ctx)


req = req.WithContext(ctx)

awly · 2018-04-26T21:30:46Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	response, err := client.Do(req)
+	if err != nil {
+		return "", err
+	}


defer response.Body.Close()

awly · 2018-04-26T21:32:57Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	}()
+
+	// Let tests wait until the verifier is initialized.
+	if synchronizeVerifierForTest {


Hmm, I think you can skip the channel and just call wait.PollUntil without a goroutine if the flag is true.

Yes, there are multiple ways of achieving this effect. I opted for a way that minimally modifies the execution path in and out of test.

ericchiang · 2018-04-26T22:34:19Z

/ok-to-test

awly · 2018-04-26T22:57:47Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+		return "", err
+	}
+	responseBytes, err := ioutil.ReadAll(response.Body)
+	defer response.Body.Close()


Move this before ReadAll. It won't make Reads fail

Is there any specific reason to prefer that idiom?

It follows the general pattern of

r, err := allocateResource() if err != nil { return err } defer cleanupResource(r)

If you keep it as is, every reader will think there's some non-obvious semantics and spend time trying to understand the reasoning behind it.

Oh, I see. I was thinking more along the lines of "the first use of response.Body is in line N". So it made sense to defer close in line N+1.

Whereas, in fact, the resource was allocated at ... := client.Do(...).

filmil · 2018-04-27T02:40:27Z

/retest

…

On Thu, Apr 26, 2018 at 6:59 PM k8s-ci-robot ***@***.***> wrote: @filmil <https://github.com/filmil>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-kops-aws 7d17c32 <7d17c32> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/63213/pull-kubernetes-e2e-kops-aws/86145/> /test pull-kubernetes-e2e-kops-aws Full PR test history <https://k8s-gubernator.appspot.com/pr/63213>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/filmil>. Please help us cut down on flakes by linking to <https://git.k8s.io/community/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPDMBDoS_exccuAnxdbiFjRh5ZYBSEsks5tsnuHgaJpZM4TnR--> .

filmil · 2018-04-27T02:40:53Z

/retest

filmil · 2018-04-27T08:35:33Z

/retest

…

On Fri, Apr 27, 2018 at 1:30 AM k8s-ci-robot ***@***.***> wrote: @filmil <https://github.com/filmil>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-e2e-kops-aws 8382197 <8382197> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/63213/pull-kubernetes-e2e-kops-aws/86227/> /test pull-kubernetes-e2e-kops-aws Full PR test history <https://k8s-gubernator.appspot.com/pr/63213>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/filmil>. Please help us cut down on flakes by linking to <https://git.k8s.io/community/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPDMOvrmr4GD19_Zl2zODxC2D-MrButks5tstcIgaJpZM4TnR--> .

ericchiang · 2018-04-27T16:24:39Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	)
+
+	if glog.V(5) {
+		glog.Infof("initial claim set: %v", c)


This is going to be really verbose... Just print the distributed claim you find in the resolve() method?

glog.Infof("resolved distributed claim %s from endpoint %s: %v", claimName, endpoint, claimValue)

Also if you print all the claims you'll print endpoint access tokens.

Yeah, I meant this to be logged only in tests, as they give insight into what happened. Let's remove since it's contentious.

ericchiang · 2018-04-27T16:30:10Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	claim string
+
+	// A HTTP transport to use for distributed claims
+	t *http.Transport


nit: this should be an http.Client

ericchiang · 2018-04-27T16:32:33Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	if av == nil {
+		// This lazy init should normally be very quick.
+		client := &http.Client{Transport: r.t, Timeout: 30 * time.Second}
+		ctx := oidc.ClientContext(context.Background(), client)


nit: would be nice if this context was cancel-able.

hm, there isn't any place to cancel it from. Any suggestions?

adding a TODO is fine for now

ericchiang · 2018-04-27T16:34:40Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+// resolve requests distributed claims from all endpoints passed in,
+// and inserts the lookup results into allClaims.
+func (r *claimResolver) resolve(endpoint endpoint, allClaims claims) error {
+	// TODO: cache resolved claims.


I'd think this has to be resolved before we turn on this functionality. These tokens are evaluated on every HTTP request. It's going to be incredibly slow if we leave this as a TODO.

Agreed, but it seemed like it would better be placed in a separate commit. What do you advise? I'm fine with adding more code to this commit, but it's already large as it is.

Add a private field to the config to let the tests enable this feature, and disable it by default?

Good idea! Done.

ericchiang · 2018-04-27T16:36:13Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go

+			w.Header().Set("Content-Type", "application/json")
+			glog.V(5).Infof("%v: returning: %+v", r.URL, *openIDConfig)
+			w.Write([]byte(*openIDConfig))
+		default:


nit: default should be a 404 with an error

ericchiang · 2018-04-27T16:45:13Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	}
+	v, err := r.Verifier(untrustedIss)
+	if err != nil {
+		glog.Errorf("verifier: %v", err)


I think it'd be better to return an error if you can't reach the distributed claim provider. Users are going to get weird authorization errors otherwise. But I'd be fine if we want to keep it this way.

ericchiang · 2018-04-27T16:46:42Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	}
+	value, ok := distClaims[r.claim]
+	if !ok {
+		return fmt.Errorf("could not find distributed claim: %v", r.claim)


We're going to live and die by good error messages here :P

fmt.Errorf("jwt returned by distributed claim endpoint %s did not contain claim: %v", endpoint, r.claim)

Agreed. Changed.

ericchiang · 2018-04-27T16:47:07Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	// TODO: Allow passing request body with configurable information.


Is this actually part of the spec?

Azure OIDC IDP supports this. IIUC the spec says nothing either way.

ericchiang · 2018-04-27T16:48:14Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

+	}
+	ep, ok := sources[src]
+	if !ok {
+		return fmt.Errorf("oidc: malformed claim sources")


fmt.Errorf("id token _claim_names contained a source %s missing in _claim_sources", src)

ericchiang · 2018-04-27T16:49:01Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

@@ -381,3 +684,11 @@ func (c claims) hasClaim(name string) bool {
 	}
 	return true
 }
+
+func (c claims) String() string {


I don't think we should ever print all the claims. It might contain sensitive information like access tokens.

ericchiang · 2018-05-01T18:06:09Z

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

@@ -104,6 +112,73 @@ type Options struct {

 	// now is used for testing. It defaults to time.Now.
 	now func() time.Time
+
+	// TODO: remove this field once caching is implemented.
+	enableDistributedClaims bool


woops, you need to remove this

ericchiang · 2018-05-01T18:06:59Z

/hold

filmil · 2018-05-01T18:13:04Z

Ow! Good catch. I do remember removing it, perhaps at some point I hit "undo" one too many times.

Please take another look @ericchiang . Sorry for the ruckus.

ericchiang · 2018-05-01T18:24:02Z

/lgtm
/hold remove

ericchiang · 2018-05-01T18:25:00Z

/hold cancel

filmil · 2018-05-01T19:35:14Z

/retest

…

On Tue, May 1, 2018 at 12:30 PM k8s-ci-robot ***@***.***> wrote: @filmil <https://github.com/filmil>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-verify bc092ee <bc092ee> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/63213/pull-kubernetes-verify/88867/> /test pull-kubernetes-verify Full PR test history <https://k8s-gubernator.appspot.com/pr/63213>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/filmil>. Please help us cut down on flakes by linking to <https://git.k8s.io/community/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPDMKLi_JXnn-LDi1Ss_Ma3fXt0HxP7ks5tuLfDgaJpZM4TnR--> .

filmil · 2018-05-01T20:54:17Z

/retest

…

On Tue, May 1, 2018 at 1:53 PM k8s-ci-robot ***@***.***> wrote: @filmil <https://github.com/filmil>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-verify bc092ee <bc092ee> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/63213/pull-kubernetes-verify/88884/> /test pull-kubernetes-verify Full PR test history <https://k8s-gubernator.appspot.com/pr/63213>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/filmil>. Please help us cut down on flakes by linking to <https://git.k8s.io/community/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPDMP0hBfthp7sdxZemytvzQkc9dfeRks5tuMtFgaJpZM4TnR--> .

A distributed claim allows the OIDC provider to delegate a claim to a separate URL. Distributed claims are of the form as seen below, and are defined in the OIDC Connect Core 1.0, section 5.6.2. See: https://openid.net/specs/openid-connect-core-1_0.html#AggregatedDistributedClaims Example claim: ``` { ... (other normal claims)... "_claim_names": { "groups": "src1" }, "_claim_sources": { "src1": { "endpoint": "https://www.example.com", "access_token": "f005ba11" }, }, } ``` Example response to a followup request to https://www.example.com is a JWT-encoded claim token: ``` { "iss": "https://www.example.com", "aud": "my-client", "groups": ["team1", "team2"], "exp": 9876543210 } ``` Apart from the indirection, the distributed claim behaves exactly the same as a standard claim. For Kubernetes, this means that the token must be verified using the same approach as for the original OIDC token. This requires the presence of "iss", "aud" and "exp" claims in addition to "groups". All existing OIDC options (e.g. groups prefix) apply. Any claim can be made distributed, even though the "groups" claim is the primary use case. Allows groups to be a single string due to kubernetes#33290, even though OIDC defines "groups" claim to be an array of strings. So, this will be parsed correctly: ``` { "iss": "https://www.example.com", "aud": "my-client", "groups": "team1", "exp": 9876543210 } ``` Expects that distributed claims endpoints return JWT, per OIDC specs. In case both a standard and a distributed claim with the same name exist, standard claim wins. The specs seem undecided about the correct approach here. Distributed claims are resolved serially. This could be parallelized for performance if needed. Aggregated claims are silently skipped. Support could be added if needed.

filmil · 2018-05-02T17:29:55Z

/retest

Let's try one more time in case this is some kind of a flake. If it repeats, I'll try to dig in.

@ericchiang any advice? For the record, the only thing I did above was to rebase on top of master to verify that there are no merge conflict. That removed the lgtm, however. Would you be able to help.

ericchiang · 2018-05-02T17:38:15Z

No great advise for the test flakes, but rebasing removing the LGTM is expected. I can re-apply the label.

/lgtm

k8s-ci-robot · 2018-05-02T17:38:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, filmil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/OWNERS~~ [ericchiang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

filmil · 2018-05-02T20:40:20Z

/test pull-kubernetes-verify

…

On Wed, May 2, 2018 at 1:38 PM k8s-ci-robot ***@***.***> wrote: @filmil <https://github.com/filmil>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-verify dfb5278 <dfb5278> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/63213/pull-kubernetes-verify/89039/> /test pull-kubernetes-verify Full PR test history <https://k8s-gubernator.appspot.com/pr/63213>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/filmil>. Please help us cut down on flakes by linking to <https://git.k8s.io/community/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPDMC5jfuPO5EnxecWdgKkf5zLBgkK3ks5tuhlKgaJpZM4TnR--> .

filmil · 2018-05-02T22:00:42Z

/test pull-kubernetes-verify :/

…

On Wed, May 2, 2018 at 2:58 PM k8s-ci-robot ***@***.***> wrote: @filmil <https://github.com/filmil>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-verify dfb5278 <dfb5278> link <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/63213/pull-kubernetes-verify/89054/> /test pull-kubernetes-verify Full PR test history <https://k8s-gubernator.appspot.com/pr/63213>. Your PR dashboard <https://k8s-gubernator.appspot.com/pr/filmil>. Please help us cut down on flakes by linking to <https://git.k8s.io/community/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/kubernetes/kubernetes/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#63213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPDMLMLAdWr2EWmv67QKLSl2pErqgHEks5tuiwHgaJpZM4TnR--> .

ericchiang · 2018-05-02T22:06:19Z

@filmil #63378

fejta-bot · 2018-05-03T01:52:03Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-05-03T02:44:53Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-05-03T03:41:50Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from caesarxuchao, ericchiang and sttts April 26, 2018 19:52

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 26, 2018

filmil force-pushed the oidc-dist-claims branch from 7e91195 to d68fd41 Compare April 26, 2018 19:57

mikedanese self-assigned this Apr 26, 2018

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 26, 2018

mikedanese assigned ericchiang and awly Apr 26, 2018

mikedanese reviewed Apr 26, 2018

View reviewed changes

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Apr 26, 2018

awly reviewed Apr 26, 2018

View reviewed changes

filmil force-pushed the oidc-dist-claims branch from d68fd41 to 9948d81 Compare April 26, 2018 22:01

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 26, 2018

awly reviewed Apr 26, 2018

View reviewed changes

filmil force-pushed the oidc-dist-claims branch from 9948d81 to 7d17c32 Compare April 27, 2018 00:58

filmil force-pushed the oidc-dist-claims branch 3 times, most recently from 85c0d8d to 8382197 Compare April 27, 2018 08:02

ericchiang reviewed Apr 27, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2018

ericchiang reviewed May 1, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2018

filmil force-pushed the oidc-dist-claims branch from 1ca7b28 to bc092ee Compare May 1, 2018 18:11

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 1, 2018

filmil force-pushed the oidc-dist-claims branch from bc092ee to dfb5278 Compare May 2, 2018 00:12

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2018

k8s-github-robot merged commit ab9f64a into kubernetes:master May 3, 2018

ericchiang mentioned this pull request May 24, 2018

Distributed claims utilities coreos/go-oidc#171

Open

liggitt mentioned this pull request Sep 28, 2022

KEP-3331: Structured Authentication Config kubernetes/enhancements#3332

Merged

enj mentioned this pull request Jun 29, 2023

[StructuredAuthenticationConfig] wire request context to claim resolver #118948

Merged

Implements OIDC distributed claims. #63213

Implements OIDC distributed claims. #63213

Conversation

filmil commented Apr 26, 2018 • edited

filmil commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmil Apr 26, 2018 • edited

Choose a reason for hiding this comment

filmil Apr 26, 2018 • edited

Choose a reason for hiding this comment

ericchiang Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmil Apr 27, 2018 • edited

Choose a reason for hiding this comment

ericchiang Apr 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Apr 26, 2018

awly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmil Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmil commented Apr 27, 2018 via email

filmil commented Apr 27, 2018

filmil commented Apr 27, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented May 1, 2018

filmil commented May 1, 2018

filmil commented Apr 26, 2018 •

edited

filmil Apr 26, 2018 •

edited

filmil Apr 26, 2018 •

edited

ericchiang Apr 26, 2018 •

edited

filmil Apr 27, 2018 •

edited

ericchiang Apr 27, 2018 •

edited

filmil Apr 26, 2018 •

edited