CiProvider as a new OIDCIssuer type #1679

javanlacerda · 2024-05-27T18:03:55Z

Contribute towards #1111

Summary

It adds CiProvider as a new OIDCIssuer type. We will migrate all ci providers to use a generic principal by changing their types to this new type.

It should not change any current behavior.

Release Note

Documentation

cc @haydentherapper

cpanato

and when you have time can you describe the PR and why we need that?

thanks!

pkg/identity/generic/issuer_test.go

javanlacerda · 2024-05-28T14:31:49Z

and when you have time can you describe the PR and why we need that?

thanks!

Sure! I apologize I hadn't do that already. I am working on this issue #1111.
My plan is having a generic module that handle ci providers id tokens claims defined in a configuration file, in this case, a yaml file.

I'll put a more detailed description in the PR summary soon.

haydentherapper · 2024-05-29T04:59:10Z

@cpanato, the motivation is to simplify CI/CD OIDC provider onboarding. Rather than have each OIDC provider have to modify code to add a new provider, they instead should only need to modify a configuration file which will contain the mapping between OIDC claim and x509 extension value.

cpanato · 2024-05-29T13:09:40Z

@cpanato, the motivation is to simplify CI/CD OIDC provider onboarding. Rather than have each OIDC provider have to modify code to add a new provider, they instead should only need to modify a configuration file which will contain the mapping between OIDC claim and x509 extension value.

that is nice! thanks for the clarification

codecov · 2024-06-06T14:22:22Z

Codecov Report

Attention: Patch coverage is 82.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 49.62%. Comparing base (cf238ac) to head (e248f43).
Report is 133 commits behind head on main.

Files	Patch %	Lines
pkg/identity/ciprovider/principal.go	80.00%	7 Missing and 7 partials ⚠️
pkg/challenges/challenges.go	0.00%	2 Missing ⚠️
pkg/identity/ciprovider/issuer.go	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1679      +/-   ##
==========================================
- Coverage   57.93%   49.62%   -8.31%     
==========================================
  Files          50       71      +21     
  Lines        3119     4181    +1062     
==========================================
+ Hits         1807     2075     +268     
- Misses       1154     1878     +724     
- Partials      158      228      +70

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/identity/generic/issuer.go

pkg/challenges/challenges.go

pkg/identity/ciprovider/principal.go

config/fulcio-config.yaml

pkg/config/config.go

pkg/identity/ciprovider/principal.go

haydentherapper · 2024-06-25T01:46:12Z

Once this PR is ready for review and all comments addressed, can you post here?

Signed-off-by: Javan lacerda <[email protected]>

go.mod

Signed-off-by: Javan lacerda <[email protected]>

pkg/config/config.go

pkg/server/issuer_pool.go

pkg/identity/ciprovider/principal.go

pkg/certificate/extensions.go

pkg/identity/ciprovider/principal.go

Signed-off-by: Javan lacerda <[email protected]>

haydentherapper

Just a few comments mostly around testing, this is coming together well!

haydentherapper · 2024-07-03T00:47:31Z

pkg/config/config.go

+	}
+
+	for _, ciIssuerMetadata := range fulcioConfig.CIIssuerMetadata {
+		v := reflect.Indirect(reflect.ValueOf(&ciIssuerMetadata.ExtensionTemplates))


Does reflect.ValueOf(ciIssuerMetadata.ExtensionTemplates) work, or do we need the indirect reflection to resolve the pointer?

Does reflect.ValueOf(ciIssuerMetadata.ExtensionTemplates) work, or do we need the indirect reflection to resolve the pointer?

Exactly, we need it for resolving the pointer

Do you need to take a pointer to ciIssuerMetadata.ExtensionTemplates though? Are reflect.ValueOf(ciIssuerMetadata.ExtensionTemplates) and reflect.Indirect(reflect.ValueOf(&ciIssuerMetadata.ExtensionTemplates)) equivalent?

It seems that reflect.ValueOf(ciIssuerMetadata.ExtensionTemplates) works. I assumed that I should use indirect for this case, as I needed to do this for using SetString. Without the indirect I got reflect.Value.SetString using unaddressable value as an error

pkg/config/config_network_test.go

haydentherapper · 2024-07-03T00:50:51Z

pkg/config/config_network_test.go

+	err = validateCIIssuerMetadata(fulcioConfig)
+	if err == nil {
+		t.Error("It should raise an error")
+	}


Did you want to test for a valid SAN template as well?

yep! just did. thanks!!

haydentherapper · 2024-07-03T00:53:50Z

pkg/identity/ciprovider/principal.go

+	}
+	uris := []*url.URL{sanURL}
+	cert.URIs = uris
+	v := reflect.Indirect(reflect.ValueOf(&claimsTemplates))


Same question here, is reflect.Indirect needed?

In this case it is. As I mentioned, getting the pointer directly by reflect.ValueOf, I have an unaddressable value. Setting it as a reference and then using the indirect was the workaround I found for this.

haydentherapper · 2024-07-03T00:59:29Z

pkg/identity/ciprovider/principal.go

+	uris := []*url.URL{sanURL}
+	cert.URIs = uris
+	v := reflect.Indirect(reflect.ValueOf(&claimsTemplates))
+	vType := v.Type()


Could you add a comment about why we need the Type, that it's needed access struct field names?

Yes! thanks

haydentherapper · 2024-07-03T01:02:02Z

pkg/identity/ciprovider/principal_test.go

+	"github.com/sigstore/fulcio/pkg/certificate"
+	"github.com/sigstore/fulcio/pkg/config"
+)
+


Are there any tests we can add for applyTemplateOrReplace, to check any edge cases?

Sure! I added the test structure just with the happy path, I'll add more tests for edge cases soon

haydentherapper · 2024-07-03T01:03:18Z

pkg/server/grpc_server_test.go

@@ -1123,6 +1131,178 @@ func TestAPIWithGitHub(t *testing.T) {
 	}
 }

+// Tests API for CiProvider subject types
+func TestAPIWithCiProvider(t *testing.T) {


Signed-off-by: Javan lacerda <[email protected]>

haydentherapper

🥳

outdated

haydentherapper · 2024-07-09T19:56:56Z

In terms of release, I'm planning to cut 1.5 from before this PR, then we'll merge this and the other related changes and cut v1.6

This reverts commit 66485b6.

This reverts commit f11344b.

This adds a new generic CI provider so that new CI providers can be added via configuration without any code changes. The existing CI providers will be migrated over. Ref: #1111 Add back #1679 Signed-off-by: Javan lacerda [email protected]

javanlacerda marked this pull request as draft May 27, 2024 18:04

javanlacerda force-pushed the javan.oidc-provider-yaml branch from e8c017c to 128fb62 Compare May 27, 2024 18:06

cpanato previously requested changes May 28, 2024

View reviewed changes

pkg/identity/generic/issuer_test.go Outdated Show resolved Hide resolved

javanlacerda force-pushed the javan.oidc-provider-yaml branch from 90e39c8 to a2a2bbc Compare May 28, 2024 17:49

javanlacerda force-pushed the javan.oidc-provider-yaml branch from fb70b39 to 6281d87 Compare June 6, 2024 14:18

javanlacerda force-pushed the javan.oidc-provider-yaml branch from 1ab3370 to 89d50e9 Compare June 6, 2024 15:29

javanlacerda changed the title ~~Drafting generic issuer implementation~~ Adding CI provider flag for OIDCIssuers Jun 6, 2024

javanlacerda marked this pull request as ready for review June 6, 2024 15:37

javanlacerda requested a review from cpanato June 6, 2024 16:27

haydentherapper reviewed Jun 11, 2024

View reviewed changes

pkg/identity/generic/issuer.go Outdated Show resolved Hide resolved

pkg/challenges/challenges.go Outdated Show resolved Hide resolved

javanlacerda force-pushed the javan.oidc-provider-yaml branch 5 times, most recently from ea867b7 to e3c0e82 Compare June 11, 2024 16:43

javanlacerda changed the title ~~Adding CI provider flag for OIDCIssuers~~ CiProvider as a new OIDCIssuer type Jun 11, 2024

javanlacerda force-pushed the javan.oidc-provider-yaml branch from 51e0ce3 to 6aec89c Compare June 14, 2024 13:38

haydentherapper requested changes Jun 17, 2024

View reviewed changes

javanlacerda force-pushed the javan.oidc-provider-yaml branch from 76bba00 to fec9d4c Compare June 19, 2024 14:43

haydentherapper requested changes Jun 21, 2024

View reviewed changes

javanlacerda force-pushed the javan.oidc-provider-yaml branch 2 times, most recently from 07c9a86 to 0a16f86 Compare June 24, 2024 18:38

javanlacerda added 3 commits June 26, 2024 12:46

drafting generic issuer implementation

486397e

Signed-off-by: Javan lacerda <[email protected]>

adding license o yaml file

f74f6a6

Signed-off-by: Javan lacerda <[email protected]>

remove yaml2.0

cc5a602

Signed-off-by: Javan lacerda <[email protected]>

pnacht reviewed Jun 26, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

remove structs usage, using mapstructure instead

13e7059

Signed-off-by: Javan lacerda <[email protected]>

javanlacerda requested review from haydentherapper and pnacht June 26, 2024 17:11

haydentherapper requested changes Jun 28, 2024

View reviewed changes

javanlacerda added 7 commits June 30, 2024 15:59

renamme IssuerMetadata

397eee3

Signed-off-by: Javan lacerda <[email protected]>

update SubjectAlternativeNameTemplate comment

04ef3c7

Signed-off-by: Javan lacerda <[email protected]>

several fixes

96a6663

Signed-off-by: Javan lacerda <[email protected]>

update for using reflect instead of mapstructure

611d5c4

Signed-off-by: Javan lacerda <[email protected]>

adding test for ciprovider API

1a481eb

Signed-off-by: Javan lacerda <[email protected]>

adding json tags for Extensions

8ab3f2a

Signed-off-by: Javan lacerda <[email protected]>

adding CIIssuerMetadata json and yaml mapping

31f0318

Signed-off-by: Javan lacerda <[email protected]>

haydentherapper reviewed Jul 3, 2024

View reviewed changes

javanlacerda mentioned this pull request Jul 3, 2024

Move configuration to yaml format #1720

Merged

javanlacerda added 4 commits July 3, 2024 20:43

fixes

b9cc94e

Signed-off-by: Javan lacerda <[email protected]>

adding test for applyTemplateOrReplace

42f803c

Signed-off-by: Javan lacerda <[email protected]>

omit CIIIssuerMetadata

478c37a

Signed-off-by: Javan lacerda <[email protected]>

adding tests case for ApplyTemplateOrReplace

7455b87

Signed-off-by: Javan lacerda <[email protected]>

javanlacerda force-pushed the javan.oidc-provider-yaml branch from ef14ccb to 7455b87 Compare July 5, 2024 21:04

update code for using Elem() instead of indirect

e248f43

Signed-off-by: Javan lacerda <[email protected]>

haydentherapper approved these changes Jul 9, 2024

View reviewed changes

haydentherapper merged commit 66485b6 into sigstore:main Jul 9, 2024
13 checks passed

haydentherapper added a commit that referenced this pull request Jul 10, 2024

Revert "CiProvider as a new OIDCIssuer type (#1679)"

2958f6f

This reverts commit 66485b6.

haydentherapper mentioned this pull request Jul 10, 2024

Revert "CiProvider as a new OIDCIssuer type" #1727

Merged

haydentherapper added a commit that referenced this pull request Jul 10, 2024

Revert "CiProvider as a new OIDCIssuer type (#1679)" (#1727)

f11344b

This reverts commit 66485b6.

haydentherapper added a commit that referenced this pull request Jul 10, 2024

Revert "Revert "CiProvider as a new OIDCIssuer type (#1679)" (#1727)"

18cd74f

This reverts commit f11344b.

haydentherapper mentioned this pull request Jul 10, 2024

CiProvider as a new OIDCIssuer type #1729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CiProvider as a new OIDCIssuer type #1679

CiProvider as a new OIDCIssuer type #1679

javanlacerda commented May 27, 2024 •

edited

Loading

cpanato left a comment

javanlacerda commented May 28, 2024 •

edited

Loading

haydentherapper commented May 29, 2024

cpanato commented May 29, 2024

codecov bot commented Jun 6, 2024 •

edited

Loading

haydentherapper commented Jun 25, 2024

haydentherapper left a comment

haydentherapper Jul 3, 2024

javanlacerda Jul 3, 2024 •

edited

Loading

haydentherapper Jul 3, 2024

javanlacerda Jul 3, 2024

haydentherapper Jul 3, 2024

javanlacerda Jul 3, 2024

haydentherapper Jul 3, 2024

javanlacerda Jul 3, 2024

haydentherapper Jul 3, 2024

javanlacerda Jul 3, 2024

haydentherapper Jul 3, 2024

javanlacerda Jul 3, 2024

haydentherapper Jul 3, 2024

haydentherapper left a comment

haydentherapper commented Jul 9, 2024

CiProvider as a new OIDCIssuer type #1679

CiProvider as a new OIDCIssuer type #1679

Conversation

javanlacerda commented May 27, 2024 • edited Loading

Summary

Release Note

Documentation

cpanato left a comment

Choose a reason for hiding this comment

javanlacerda commented May 28, 2024 • edited Loading

haydentherapper commented May 29, 2024

cpanato commented May 29, 2024

codecov bot commented Jun 6, 2024 • edited Loading

Codecov Report

haydentherapper commented Jun 25, 2024

haydentherapper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanlacerda Jul 3, 2024 • edited Loading

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

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper commented Jul 9, 2024

javanlacerda commented May 27, 2024 •

edited

Loading

javanlacerda commented May 28, 2024 •

edited

Loading

codecov bot commented Jun 6, 2024 •

edited

Loading

javanlacerda Jul 3, 2024 •

edited

Loading