Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bunert)

scion-pki/key/fingerprint.go line 56 at r1 (raw file):

in the file.

The subject key id is written to standard out.

To me this description suggested that this shows "the fingerprint of the subject key id of a public key", which rather confused me. I believe "subject key id" is just a specific name for this fingerprint of the public key. Perhaps this could be reworded a bit to make this clearer.

Similarly, the description of the "full-key-digest" leaves me wondering what the difference is to the other digest type is.

Btw., I've just had a look at the description of (step crypto key fingerprint)[https://smallstep.com/docs/step-cli/reference/crypto/key/fingerprint], which seems a bit clearer on these points (assuming this really is the equivalent command).

scion-pki/key/fingerprint.go line 60 at r1 (raw file):

		Args: cobra.ExactArgs(1),
		RunE: func(cmd *cobra.Command, args []string) error {
			cmd.SilenceUsage = true

Nit. With a wrong format option, the help text will not be shown. Not sure if this is good or bad, but it's different from scion-pki certificate fingerprint.

scion-pki/key/fingerprint.go line 145 at r1 (raw file):

// encodeSubjectKeyID encodes the subject key id in provided format:
// hex, base64, base64-url, base64-raw, base64-url-raw, emoji

Should the same formatting options also be added to scion-pki certificate fingerprint, e.g. by moving this into the encoding package?

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matzf)

scion-pki/key/fingerprint.go line 56 at r1 (raw file):

I adapted the description and tried to fix the confusion about the full-key-digest flag. Now the description should be formulated consistently to avoid confusion.

scion-pki/key/fingerprint.go line 60 at r1 (raw file):

Now similar to scion-pki certificate fingerprint.

scion-pki/key/fingerprint.go line 145 at r1 (raw file):

Yeah good point, adjusted it.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @bunert)

scion-pki/key/fingerprint.go line 56 at r1 (raw file):

💯

Reviewed 5 of 6 files at r1, 4 of 4 files at r2.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @bunert)

scion-pki/certs/fingerprint.go line 51 at r2 (raw file):

		Args: cobra.ExactArgs(1),
		RunE: func(cmd *cobra.Command, args []string) error {
			err := encoding.CheckEncodings(flags.format)

Always inline the assignment in the if statements when possible.
This reduces the scope of the error variable and makes referencing the wrong variable by mistake harder to do.

Suggestion:

			if err := encoding.CheckEncodings(flags.format); err != nil {
				return err
			}

scion-pki/encoding/encoding.go line 14 at r2 (raw file):

// See the License for the specific language governing permissions and
// limitations under the License.
package encoding

Add a new line between the license header and the package declaration.
Otherwise, godoc will recognize this as the package doc comment.

scion-pki/encoding/encoding.go line 24 at r2 (raw file):

)

func CheckEncodings(format string) error {

Add a doc comment.

scion-pki/key/fingerprint.go line 34 at r2 (raw file):

// NewFingerprintCmd returns a cobra command that returns the subject key id of a
// public key. If a private key is given, the subject key id is computed for the

Suggestion:

// NewFingerprintCmd returns a cobra command that returns the subject key ID of a
// public key. If a private key is given, the subject key ID is computed for the
// corresponding public key. For certificated or certificates chains, the subject
// key ID is computed with respect to the public key of the first certificate in the file.

scion-pki/key/fingerprint.go line 53 at r2 (raw file):

in the file.

By default the fingerprint calculated is SHA-1 hash of the marshaled public key defined in

Suggestion:

By default the fingerprint calculated is SHA-1 hash of the marshaled public key as defined in
https://tools.ietf.org/html/rfc5280#section-4.2.1.2 (1). With the '--full-key-digest' flag,
the computed fingerprint is the SHA-1 hash with ASN.1 DER-encoded subjectPublicKey.

scion-pki/key/fingerprint.go line 57 at r2 (raw file):

the computed fingerprint is the SHA-1 hash with ASN.1 DER-encoded subjectPublicKey.

The subject key id is written to standard out.

Suggestion:

ID

scion-pki/key/fingerprint.go line 88 at r2 (raw file):

			output, err := encoding.EncodeBytes(skid, flags.format)
			if err != nil {
				return serrors.WrapStr("encoding subject key id", err)

Suggestion:

ID"

scion-pki/key/fingerprint.go line 104 at r2 (raw file):

// LoadPublicKey loads the public key from file and distinguishes what type of key it is.
func LoadPublicKey(filename string) (crypto.PublicKey, error) {

This does not appear to be used anywhere else. Make it private.

Be vigilant when extending the API surface of a package. You will enter a contract that you will need to support that function for ever.
Changing or removing it will be considered a breaking change.

Once we need it somewhere else, we can always make it public.

scion-pki/key/fingerprint.go line 107 at r2 (raw file):

	raw, err := os.ReadFile(filename)
	if err != nil {
		return nil, serrors.WrapStr("reading input key", err)

Suggestion:

reading input file

scion-pki/key/fingerprint.go line 122 at r2 (raw file):

		pub, ok := key.(crypto.Signer)
		if !ok {
			return nil, serrors.New("cannot get public key from private key",

Suggestion:

unsupported private key type

scion-pki/key/fingerprint.go line 136 at r2 (raw file):

		cert, err := x509.ParseCertificate(block.Bytes)
		if err != nil {
			return nil, serrors.WrapStr("error parsing certificate", err)

Suggestion:

parsing certificate

scion-pki/key/fingerprint.go line 141 at r2 (raw file):

	default:
		return nil, serrors.New(
			"file is not a valid PEM encoding of a private/public key or certificate", "type",

Suggestion:

		return nil, serrors.New("unsupported PEM block", "type", block.Type)

scion-pki/key/fingerprint_test.go line 132 at r2 (raw file):

			cmd := key.NewFingerprintCmd(command.StringPather("test"))

			// TODO: fullkeydigest flag

Drop the todo

scion-pki/key/fingerprint_test.go line 149 at r2 (raw file):

		})
	}

drop the new line

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @oncilla)

scion-pki/certs/fingerprint.go line 51 at r2 (raw file):

Done.

scion-pki/encoding/encoding.go line 14 at r2 (raw file):

Done.

scion-pki/encoding/encoding.go line 24 at r2 (raw file):

Done.

scion-pki/key/fingerprint.go line 34 at r2 (raw file):

// NewFingerprintCmd returns a cobra command that returns the subject key id of a
// public key. If a private key is given, the subject key id is computed for the

Done.

scion-pki/key/fingerprint.go line 53 at r2 (raw file):

in the file.

By default the fingerprint calculated is SHA-1 hash of the marshaled public key defined in

Done.

scion-pki/key/fingerprint.go line 57 at r2 (raw file):

the computed fingerprint is the SHA-1 hash with ASN.1 DER-encoded subjectPublicKey.

The subject key id is written to standard out.

Done.

scion-pki/key/fingerprint.go line 88 at r2 (raw file):

			output, err := encoding.EncodeBytes(skid, flags.format)
			if err != nil {
				return serrors.WrapStr("encoding subject key id", err)

Done.

scion-pki/key/fingerprint.go line 104 at r2 (raw file):

Done.

scion-pki/key/fingerprint.go line 107 at r2 (raw file):

	raw, err := os.ReadFile(filename)
	if err != nil {
		return nil, serrors.WrapStr("reading input key", err)

Done.

scion-pki/key/fingerprint.go line 122 at r2 (raw file):

		pub, ok := key.(crypto.Signer)
		if !ok {
			return nil, serrors.New("cannot get public key from private key",

Done.

scion-pki/key/fingerprint.go line 136 at r2 (raw file):

		cert, err := x509.ParseCertificate(block.Bytes)
		if err != nil {
			return nil, serrors.WrapStr("error parsing certificate", err)

Done.

scion-pki/key/fingerprint.go line 141 at r2 (raw file):

	default:
		return nil, serrors.New(
			"file is not a valid PEM encoding of a private/public key or certificate", "type",

Done.

scion-pki/key/fingerprint_test.go line 132 at r2 (raw file):

Done.

scion-pki/key/fingerprint_test.go line 149 at r2 (raw file):

Done.

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @bunert)