Default generated PEM labels to SIGSTORE

[ivanayov]

Summary

This change adds "SIGSTORE" PEM label and accepts both the old "COSIGN" label and the new one.
Newly generated keys are now created with "SIGSTORE" label

Resolves #2471

As #2376 is not closed yet, this change updates to generating private keys with "SIGSTORE" PEM label by default.

Did not update the CI (in particular .github/workflows/cosign-test.key) for backwards compatibility safety (i.e. still available "COSIGN" label support).

Release Note

Generated PEM labels were updated to "SIGSTORE" rather than the previous "COSIGN". Both are still supported and accepted, but newly generated will be with the updated label.

Documentation

PEM labels were not previously referenced in the docs, so nothing to update.

[znewman01]

Sorry for the slow review! I should be quicker in the future.

Can we add a test case to make sure that both are read successfully?

One for writing too would be good.

[ivanayov]

Thanks @znewman01 !

I added tests for reading both the old and the new type, and writing the new type, as the current change defaults to "SIGSTORE".

You mentioned an option to emit the old "COSIGN" type for backwards compatibility and I have some questions:

• As both are accepted successfully now, do we consider it's breaking the backwards compatibility for not emitting "COSIGN"? (To me it looks like No, but you might know some use-cases where people would still need generating the old type.)
• Supporting both would mean giving a cmd or config options to select from and I have some concerns here:
  • Generating key pair at the moment is pretty simple and straightforward and adding one more step for something that's usually auto-generated might look confusing
  • Allowing an option to select a PEM label looks a bit strange from user perspective and I think if we do that, there should be some strong reasoning behind
  • I had a look and it requires some refactoring - not big, but touching generic functionality. I'm ok with addressing that if you think it's worth and not risky for a temporary change (considering it's going to be removed in several months)

This is why the current change emits only "SIGSTORE", but I'm happy to add support for both if you think that would be the right approach.

Thank you for your input in advance.

[znewman01]

As both are accepted successfully now, do we consider it's breaking the backwards compatibility for not emitting "COSIGN"? (To me it looks like No, but you might know some use-cases where people would still need generating the old type.)

The question is: are we worried about scenarios where a new client generates a key and an older client uses that key?

I can see an argument for "no" because this is mostly a client-side concern, but I worry about public keys generated by new clients.

That said--I'd be okay omitting the option for now for the other reasons you mention.

[znewman01]

If you fix lint I'm happy with this!

[ivanayov]

Thanka @znewman01, I pushed an update

[codecov]

Codecov Report

Merging #2735 (dfbb56c) into main (9d55c5e) will decrease coverage by 0.01%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main    #2735      +/-   ##
==========================================
- Coverage   29.54%   29.54%   -0.01%     
==========================================
  Files         151      151              
  Lines        9646     9658      +12     
==========================================
+ Hits         2850     2853       +3     
- Misses       6357     6366       +9     
  Partials      439      439              

Impacted Files	Coverage Δ
pkg/cosign/keys.go	59.25% <87.50%> (+0.92%)	⬆️

... and 10 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.