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 client credential flow #1620

Merged
merged 5 commits into from Feb 15, 2024
Merged

add client credential flow #1620

merged 5 commits into from Feb 15, 2024

Conversation

nkreiger
Copy link
Contributor

@nkreiger nkreiger commented Feb 5, 2024

Summary

This PR adds client credentials as a supported OIDC Auth Flow Provider.

Closes #1619

Release Note

Adds a new oauthflow type, client_credentials that can be used by the consumers of the sigstore package

Documentation

The change in the cosign CLI probably would? Let me know on this.

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Overall LGTM - Have you been added to add this to a fork of Cosign to confirm this works for a private OIDC provider?

pkg/oauthflow/client_credentials_test.go Outdated Show resolved Hide resolved
@nkreiger
Copy link
Contributor Author

nkreiger commented Feb 5, 2024

Overall LGTM - Have you been added to add this to a fork of Cosign to confirm this works for a private OIDC provider?

@haydentherapper I have! And I've been testing it live since Friday. Working as expected

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
@nkreiger
Copy link
Contributor Author

nkreiger commented Feb 5, 2024

@haydentherapper is there something I'm missing for the unit tests? I checked the tests I added locally and they are working

@haydentherapper
Copy link
Contributor

There’s a lint failure.

Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
@nkreiger
Copy link
Contributor Author

nkreiger commented Feb 5, 2024

@haydentherapper all issues resolved 👍 thanks!

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

Do you know if https://pkg.go.dev/golang.org/x/oauth2/clientcredentials could be used to simplify some of the code here?

@nkreiger
Copy link
Contributor Author

nkreiger commented Feb 5, 2024

Do you know if https://pkg.go.dev/golang.org/x/oauth2/clientcredentials could be used to simplify some of the code here?

@haydentherapper it would really just replace this portion:

resp, err := http.PostForm(codeURL, data)
	if err != nil {
		return "", err
	}
	defer resp.Body.Close()

	b, err := io.ReadAll(resp.Body)
	if err != nil {
		return "", err
	}
	if resp.StatusCode != http.StatusOK {
		return "", fmt.Errorf("%s: %s", resp.Status, b)
	}

	tr := tokenResp{}
	if err := json.Unmarshal(b, &tr); err != nil {
		return "", err
	}

This is lined up to match the code for the other flows, so is shares the same test structure and consistency.

I think we could utilize the OAuth package overall across to simplify some of the code, but it should probably be used for all the flows I would think to keep that consistency? Thoughts?

@haydentherapper
Copy link
Contributor

It's not a blocker for the PR, I agree that cleaning up all of the existing flows at once would be best.

@nkreiger
Copy link
Contributor Author

nkreiger commented Feb 5, 2024

@haydentherapper I can also make the change on the cosign side, what is the process for that? This is merged in and a new release is cut?

@haydentherapper
Copy link
Contributor

Yea, I'll wait ~24 hours to see if there's any other comments from other maintainers, then merge and cut a release. Feel free to put up a draft PR for Cosign replacing sigstore/sigstore with a fork in go.mod in the meantime.

@haydentherapper haydentherapper merged commit 922069a into sigstore:main Feb 15, 2024
9 checks passed
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.

OAuth Flow support for Grant Type Client Credentials
2 participants