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

bump github.com/coreos/go-oidc dep to pick up nbf enforcement #81413

Merged
merged 1 commit into from Aug 31, 2019

Conversation

@anderseknert
Copy link
Contributor

commented Aug 14, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
If the nbf (not before) claim is present in an ID token, check that it's indeed in the past. This allows the token issuer to control the point in time at when the token should be usable for authentication.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The nbf (not before) claim, if present in ID token, is now enforced.  

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Welcome @anderseknert!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Hi @anderseknert. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ericchiang

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

/ok-to-test

}

nbfTime := time.Unix(nbfValue, 0)
now := a.now()

This comment has been minimized.

Copy link
@ericchiang

ericchiang Aug 14, 2019

Member

now() can be nil here. This needs to default to time.Now above.

This comment has been minimized.

Copy link
@anderseknert

anderseknert Aug 14, 2019

Author Contributor

Ah, alright - golang new to me so still trying to figure out all the conditions for when nil checks are needed. I thought the check at line 308 would be enough, but coming to think of it of course that only covers the case where the authenticator is constructed by the newAuthenticator function. Any good tools for finding possible nil-problems like there is for Java? I'll fix.

This comment has been minimized.

Copy link
@ericchiang

ericchiang Aug 14, 2019

Member

I don't know of a common one. Generally, you ensure this during initialization:

now := config.now
if now == nil {
    now = time.Now
}
a := authenticator{
    now: now,
    // ...
}

This comment has been minimized.

Copy link
@mikedanese

mikedanese Aug 15, 2019

Member

Or a := authenticator{now: time.Now} and set over it in tests.

@ericchiang

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

This might be a better feature request for github.com/coreos/go-oidc, since that's what does the current expiration validation.

@anderseknert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

I considered that too, but nbf doesn't make much sense outside of authentication contexts IMO - and as that library is more generic I thought it would fit better here. If you think I should try submitting a PR there first I'll do that.

@ericchiang

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I think nbf makes sense for ID Token validation in go-oidc instead of having it only in Kubernetes. Want to submit it there, then we can pull it into kube?

cc @mikedanese

Copy link
Member

left a comment

Adding this to go-oidc makes sense to me as well. Can you send a change to https://github.com/coreos/go-oidc, we'll review there, and then we'll bump the dep?

}

nbfTime := time.Unix(nbfValue, 0)
if now.Before(nbfTime) {

This comment has been minimized.

Copy link
@mikedanese

mikedanese Aug 15, 2019

Member

We should add some leeway here. A minute seems good.

}

nbfTime := time.Unix(nbfValue, 0)
now := a.now()

This comment has been minimized.

Copy link
@mikedanese

mikedanese Aug 15, 2019

Member

Or a := authenticator{now: time.Now} and set over it in tests.

@anderseknert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Thanks guys! PR targeting go-oidc coming up 👍

@fedebongio

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

/remove-sig api-machinery

@mikedanese

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

@anderseknert do you want to bump the go-oidc dependency?

@anderseknert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

I'd be delighted @mikedanese . Just heading to bed here but will check it out first thing tomorrow after having read up on how that works, hah. It's been three weeks since I wrote my first line of Go so it's all been moving pretty fast. Any pointers would be appreciated :)

@mikedanese

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

updating deps in kubernetes is no easy task. will give it a shot and report instructions.

Edit: short answer is it's really hard. Maybe there are some docs I don't know about.

go.mod Outdated
@@ -219,7 +219,7 @@ replace (
github.com/coreos/bbolt => github.com/coreos/bbolt v1.3.1-coreos.6
github.com/coreos/etcd => github.com/coreos/etcd v3.3.13+incompatible
github.com/coreos/go-etcd => github.com/coreos/go-etcd v2.0.0+incompatible
github.com/coreos/go-oidc => github.com/coreos/go-oidc v0.0.0-20180117170138-065b426bd416
github.com/coreos/go-oidc => github.com/coreos/go-oidc v2.0.1-0.20190815175729-2be1c5b8a260+incompatible

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 19, 2019

Member

I won't stick on it since we weren't on a tag before, but any chance of getting a v2.1.0 tag cut for this? cc @ericchiang

This comment has been minimized.

Copy link
@ericchiang

ericchiang Aug 19, 2019

Member

Yeah. I'll do that today

This comment has been minimized.

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Edit: short answer is it's really hard. Maybe there are some docs I don't know about.

See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/vendor.md#adding-or-updating-a-dependency
or

kubernetes/go.mod

Lines 1 to 3 in f4521bf

// This is a generated file. Do not edit directly.
// Run hack/pin-dependency.sh to change pinned dependency versions.
// Run hack/update-vendor.sh to update go.mod files and the vendor directory.

@mikedanese mikedanese force-pushed the Bisnode:OIDC-honour-nbf branch from 792ba55 to 84f18e3 Aug 20, 2019
@anderseknert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

/retest

@anderseknert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

That's a great resource, thanks @liggitt! Will try to do it myself next time. About the failing tests reported, I don't see how they are related to any changes made here. Are they known to be flaky, or something else that changed? I'm on a 2 week leave so a little hard trying to follow when not on my computer. Again - thanks for all the help guys!

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 20, 2019
@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

unfortunately, yes, sometimes the scale tests flake

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anderseknert, liggitt

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:

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

@anderseknert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

Anything more needed from me here?

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Anything more needed from me here?

it needs a rebase on master (I'd recommend running hack/update-vendor.sh after rebase to make sure generated files are up to date as well)

@dims

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

/milestone v1.16

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 26, 2019
@anderseknert anderseknert force-pushed the Bisnode:OIDC-honour-nbf branch from 84f18e3 to 5e6162c Aug 31, 2019
@anderseknert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@liggitt I've now rebased, fixed conflicts and run hack/update-vendor.sh which checked out (thanks for the pointer, btw). As the conflicting changes are in go.mod|sum though I expect this will diverge from master soon again though.

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

Thanks!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 31, 2019
@k8s-ci-robot k8s-ci-robot merged commit 443002f into kubernetes:master Aug 31, 2019
23 of 24 checks passed
23 of 24 checks passed
tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation anderseknert authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.