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

Infra flags for fulcio / rekor / oidc values #462

Merged
merged 8 commits into from
Jul 22, 2021
Merged

Infra flags for fulcio / rekor / oidc values #462

merged 8 commits into from
Jul 22, 2021

Conversation

lukehinds
Copy link
Member

Implements flags for fulco, rekor and OIDC values to allow a user to set their own custom values.

The defaults fall to the public good service

image

image

Luke Hinds added 2 commits July 21, 2021 12:54
This change allows users to specify their own values for:

* Fulcio
* Rekor
* OIDC Issuer
* OIDC Client ID
* OIDC Secret

All flags are marked as experimental

Signed-off-by: Luke Hinds <lhinds@redhat.com>
Signed-off-by: Luke Hinds <lhinds@redhat.com>
@cpanato cpanato added this to the v1.0.0 milestone Jul 21, 2021
@dlorenc
Copy link
Member

dlorenc commented Jul 21, 2021

Looks like you might need to plumb the flag through to the e2e tests to point at localhost

Luke Hinds added 3 commits July 21, 2021 15:03
Signed-off-by: Luke Hinds <lhinds@redhat.com>
Signed-off-by: Luke Hinds <lhinds@redhat.com>
Signed-off-by: Luke Hinds <lhinds@redhat.com>
@@ -50,6 +51,7 @@ func applyVerifyFlags(cmd *VerifyCommand, flagset *flag.FlagSet) {
flagset.StringVar(&cmd.KeyRef, "key", "", "path to the public key file, URL, KMS URI or Kubernetes Secret")
flagset.BoolVar(&cmd.Sk, "sk", false, "whether to use a hardware security key")
flagset.StringVar(&cmd.Slot, "slot", "", "security key slot to use for generated key (default: signature) (authentication|signature|card-authentication|key-management)")
flagset.StringVar(&cmd.RekorURL, "rekor-url", "https://rekor.sigstore.dev", "address of rekor STL server")
Copy link
Member

Choose a reason for hiding this comment

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

s/rekor-url/rekor-server/

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly prefer url, as it is a url. Are we using rekor-server anywhere else in cosign?

Copy link
Member

Choose a reason for hiding this comment

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

rekor-server was used everywhere else in this PR except here, so I assumed that's what you intended here as well.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should probably s/rekor-server/rekor-url/

Copy link
Member

Choose a reason for hiding this comment

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

+1 to using rekor-url.

@@ -165,7 +153,7 @@ type Signer struct {
*signature.ECDSASignerVerifier
}

func NewSigner(ctx context.Context, idToken string) (*Signer, error) {
func NewSigner(ctx context.Context, idToken, oidcIssuer string, oidcClientID string, oidcClientSecret string, fulcioClient string, rekorClient string) (*Signer, error) {
Copy link
Member

Choose a reason for hiding this comment

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

You can combine all these argument names and use one string argument type e.g. oidIssuer, oidcClientID, oidcClientSecret, fulcioClient, rekorClient string to be more Go idiomatic. This comment applies above as well.

Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to keep these named fulcioURL and rekorURL for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can combine all these argument names and use one string argument type e.g. oidIssuer, oidcClientID, oidcClientSecret, fulcioClient, rekorClient string to be more Go idiomatic. This comment applies above as well.

Yup, good call.

Any reason not to keep these named fulcioURL and rekorURL for consistency?

I think I was sticking to the convention already set in the struct, I will have check.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be in favor of changing these to "URL" or "Endpoint" to be more descriptive

Copy link
Member Author

@lukehinds lukehinds Jul 22, 2021

Choose a reason for hiding this comment

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

Looking into it we don't need rekorClient there(so removed it) and the fulcioClient is an connection instance from getFulcioClient

Luke Hinds added 3 commits July 22, 2021 08:25
Signed-off-by: Luke Hinds <lhinds@redhat.com>
Signed-off-by: Luke Hinds <lhinds@redhat.com>
Signed-off-by: Luke Hinds <lhinds@redhat.com>
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.

5 participants