feat: add GPG/OpenPGP signing support module (bindings/go/gpg)#2560
Conversation
✅ Deploy Preview for ocm-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds native GPG/OpenPGP signing: a Go signing.Handler (Sign/Verify), typed GPG credentials and identity, signing configuration and schemas, credential loaders, comprehensive tests, module/Taskfile wiring, wordlist updates, and an ADR documenting design and usage. ChangesGPG/OpenPGP Signing Feature
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
78ded31 to
691d4ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
bindings/go/gpg/spec/signing/v1alpha1/schemas/Config.schema.json (1)
13-16: ⚡ Quick winEnforce
keyFingerprintformat in schema.The description constrains accepted values, but the schema currently accepts any string. Add a regex pattern so validation matches the documented contract.
Suggested diff
"keyFingerprint": { "type": "string", + "pattern": "^(?:[A-Fa-f0-9]{40}|[A-Fa-f0-9]{16})$", "description": "KeyFingerprint pins which key in the keyring to use when signing or verifying.\nWhen empty the first available key is used.\nAccepts a full 40-hex-character v4 fingerprint or a 16-hex-character long key ID." },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/go/gpg/spec/signing/v1alpha1/schemas/Config.schema.json` around lines 13 - 16, Update the Config.schema.json schema for the keyFingerprint property to enforce the documented formats by adding a regex pattern to the "keyFingerprint" definition; the pattern should allow either a 40-hex-character v4 fingerprint or a 16-hex-character key ID (and permit empty string if empty values are allowed), e.g. use a pattern like "^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{16})?$" so validation matches the description for keyFingerprint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/go/gpg/signing/handler/handler.go`:
- Around line 61-74: The code calls gpgcredentials.PrivateEntityFromCredentials
which returns a single entity and then runs selectEntityByFingerprint against a
one-element list, so fingerprint selection never finds non-first keys; change
the flow to load all private entities from the credentials (e.g., a function
that returns an openpgp.EntityList or iterate the keyring in the parsed armored
block) instead of a single entity, then call selectEntityByFingerprint against
that EntityList (or loop entities and compare fingerprints) and return
ErrMissingPrivateKey if no matching entity is found; update uses of the local
variable entity and the fingerprint branch (sigCfg.GetKeyFingerprint()) to
operate on the selected entity from the multi-entity list and preserve existing
error wrapping.
In `@bindings/go/gpg/signing/handler/internal/credentials/credentials.go`:
- Around line 21-22: The exported helpers PrivateEntityFromCredentials and
PublicEntityFromCredentials currently dereference the input pointer immediately
and will panic on nil; add a nil guard at the start of each function (check if
creds == nil) and return a clear error (e.g., fmt.Errorf("nil credentials"))
before any use of creds, then proceed to call loadBytes and the rest of the
logic—update both PrivateEntityFromCredentials and PublicEntityFromCredentials
accordingly.
In `@docs/adr/0023_gpg_signing.md`:
- Around line 46-47: The ADR claims the new Go module
ocm.software/open-component-model/bindings/go/gpg is already registered/shipped
in the ocm CLI; change those statements (the sentences describing the gpg
signing.Handler registration and shipping) to clearly mark them as
future/planned integration (e.g., "will be registered" or "planned to be
registered in the ocm CLI after module tagging and the follow-up PR"), and apply
the same rephrasing to the other similar occurrence referring to CLI shipping;
keep references to the module and ProtonMail OpenPGP library but avoid asserting
the registration is already shipped.
- Line 61: Replace the inconsistent CLI example that uses "componentversion"
with the actual command name "component-version" so all examples match and
copy/paste correctly; update the examples where the string "ocm sign
componentversion --signer-spec ./gpg.yaml" (and any other occurrences of
"componentversion") appears to "ocm sign component-version --signer-spec
./gpg.yaml", ensuring the examples at the earlier example and the ones currently
showing "ocm sign component-version --signer-spec ./gpg.yaml" are consistent.
---
Nitpick comments:
In `@bindings/go/gpg/spec/signing/v1alpha1/schemas/Config.schema.json`:
- Around line 13-16: Update the Config.schema.json schema for the keyFingerprint
property to enforce the documented formats by adding a regex pattern to the
"keyFingerprint" definition; the pattern should allow either a 40-hex-character
v4 fingerprint or a 16-hex-character key ID (and permit empty string if empty
values are allowed), e.g. use a pattern like
"^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{16})?$" so validation matches the description
for keyFingerprint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b787964b-ef8a-44d7-a985-40577932d834
⛔ Files ignored due to path filters (1)
bindings/go/gpg/go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.github/config/wordlist.txtTaskfile.ymlbindings/go/gpg/Taskfile.ymlbindings/go/gpg/go.modbindings/go/gpg/signing/handler/handler.gobindings/go/gpg/signing/handler/handler_test.gobindings/go/gpg/signing/handler/internal/credentials/credentials.gobindings/go/gpg/signing/handler/internal/credentials/credentials_test.gobindings/go/gpg/spec/credentials/v1alpha1/gpg_credentials.gobindings/go/gpg/spec/credentials/v1alpha1/gpg_credentials_test.gobindings/go/gpg/spec/credentials/v1alpha1/schemas/GPGCredentials.schema.jsonbindings/go/gpg/spec/credentials/v1alpha1/zz_generated.deepcopy.gobindings/go/gpg/spec/credentials/v1alpha1/zz_generated.ocm_jsonschema.gobindings/go/gpg/spec/credentials/v1alpha1/zz_generated.ocm_type.gobindings/go/gpg/spec/identity/v1alpha1/register.gobindings/go/gpg/spec/identity/v1alpha1/schemas/GPGIdentity.schema.jsonbindings/go/gpg/spec/identity/v1alpha1/type.gobindings/go/gpg/spec/identity/v1alpha1/type_test.gobindings/go/gpg/spec/identity/v1alpha1/zz_generated.deepcopy.gobindings/go/gpg/spec/identity/v1alpha1/zz_generated.ocm_jsonschema.gobindings/go/gpg/spec/identity/v1alpha1/zz_generated.ocm_type.gobindings/go/gpg/spec/signing/v1alpha1/algorithm.gobindings/go/gpg/spec/signing/v1alpha1/config.gobindings/go/gpg/spec/signing/v1alpha1/group_version.gobindings/go/gpg/spec/signing/v1alpha1/schemas/Config.schema.jsonbindings/go/gpg/spec/signing/v1alpha1/zz_generated.deepcopy.gobindings/go/gpg/spec/signing/v1alpha1/zz_generated.ocm_jsonschema.gobindings/go/gpg/spec/signing/v1alpha1/zz_generated.ocm_type.godocs/adr/0023_gpg_signing.md
e33de5c to
58948c2
Compare
frewilhelm
left a comment
There was a problem hiding this comment.
Does it make sense to already add the module to the website https://github.com/open-component-model/open-component-model/tree/main/website/static/open-component-model/bindings/go?
I would like to do this together with the CLI integration as otherwise the bindings are unusable |
Don't you need to import the module for the CLI integration? I believed we need the change on the website to make the module visible for go's dependency management. |
These are 2 PRs. There is bindings/go/gpg , and then there is cli. If i wanna push this here, I would have to create docs without having it ready and use a go.work with a draft PR. |
11f777b to
8abc01d
Compare
68cf355 to
b5131ab
Compare
Aligns with RSA credential pattern: move key name constants to private and extract FromDirectCredentials into a separate convert.go file. Tests use string literals instead of exported constants. Signed-off-by: Jakob Möller <contact@jakob-moeller.com> On-behalf-of: @SAP <jakob.moeller@sap.com>
- Update Sign/Verify to accept runtime.Typed credentials (matches signing.Handler interface from gate 7 migration) - Add ConvertToGPGCredentials with fast-path type assertion before scheme lookup - Rewrite handler tests to use *gpgcredentialsv1.GPGCredentials directly - Add bindings/go/credentials as direct dependency Signed-off-by: Jakob Möller <contact@jakob-moeller.com> On-behalf-of: @SAP <jakob.moeller@sap.com>
3beb3a7 to
b97020f
Compare
Signed-off-by: Jakob Möller <contact@jakob-moeller.com> On-behalf-of: @SAP <jakob.moeller@sap.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/go/gpg/spec/credentials/v1alpha1/convert.go (1)
12-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCredential property keys appear to violate the documented contract.
These constants use camelCase (
privateKeyPGP, etc.), while the PR contract defines snake_case keys (private_key_pgp,private_key_pgp_file,public_key_pgp,public_key_pgp_file,passphrase). If users provide the documented keys, conversion on Lines 67-71 will silently yield empty fields.Proposed fix
- credentialKeyPrivateKeyPGP = "privateKeyPGP" - credentialKeyPrivateKeyPGPFile = "privateKeyPGPFile" - credentialKeyPublicKeyPGP = "publicKeyPGP" - credentialKeyPublicKeyPGPFile = "publicKeyPGPFile" + credentialKeyPrivateKeyPGP = "private_key_pgp" + credentialKeyPrivateKeyPGPFile = "private_key_pgp_file" + credentialKeyPublicKeyPGP = "public_key_pgp" + credentialKeyPublicKeyPGPFile = "public_key_pgp_file" credentialKeyPassphrase = "passphrase"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/go/gpg/spec/credentials/v1alpha1/convert.go` around lines 12 - 16, The credential key constants (credentialKeyPrivateKeyPGP, credentialKeyPrivateKeyPGPFile, credentialKeyPublicKeyPGP, credentialKeyPublicKeyPGPFile, credentialKeyPassphrase) currently hold camelCase strings but must use the documented snake_case keys; update their string values to "private_key_pgp", "private_key_pgp_file", "public_key_pgp", "public_key_pgp_file", and "passphrase" respectively so the conversion logic that reads those keys produces the expected non-empty fields and remains consistent across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/go/gpg/spec/credentials/v1alpha1/convert.go`:
- Around line 33-40: The ConvertToGPGCredentials function can panic when creds
is nil because it calls creds.GetType(); add a nil guard at the start of
ConvertToGPGCredentials to check if creds == nil and return a clear typed error
(e.g., fmt.Errorf("nil credentials provided")) instead of proceeding; then
continue with the existing type assertion and
convertScheme.NewObject(creds.GetType()) logic unchanged so downstream code
never dereferences a nil creds.
---
Outside diff comments:
In `@bindings/go/gpg/spec/credentials/v1alpha1/convert.go`:
- Around line 12-16: The credential key constants (credentialKeyPrivateKeyPGP,
credentialKeyPrivateKeyPGPFile, credentialKeyPublicKeyPGP,
credentialKeyPublicKeyPGPFile, credentialKeyPassphrase) currently hold camelCase
strings but must use the documented snake_case keys; update their string values
to "private_key_pgp", "private_key_pgp_file", "public_key_pgp",
"public_key_pgp_file", and "passphrase" respectively so the conversion logic
that reads those keys produces the expected non-empty fields and remains
consistent across the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a6cf8c5-e407-40d5-8424-a5cb1468d2f0
⛔ Files ignored due to path filters (1)
bindings/go/gpg/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
bindings/go/gpg/go.modbindings/go/gpg/signing/handler/handler.gobindings/go/gpg/signing/handler/handler_test.gobindings/go/gpg/spec/credentials/v1alpha1/convert.gowebsite/static/open-component-model/bindings/go/gpg
🚧 Files skipped from review as they are similar to previous changes (3)
- bindings/go/gpg/go.mod
- bindings/go/gpg/signing/handler/handler_test.go
- bindings/go/gpg/signing/handler/handler.go
Signed-off-by: Jakob Möller <contact@jakob-moeller.com> On-behalf-of: @SAP <jakob.moeller@sap.com>
## Summary Adds the `bindings/go/gpg` module implementing GPG/OpenPGP signing support for OCM component versions. Closes open-component-model/ocm#1544 - New standalone Go module `ocm.software/open-component-model/bindings/go/gpg` - Uses `github.com/ProtonMail/go-crypto` (already a transitive dep in CLI) - Passphrase-protected private keys supported; key decrypted in-memory only, never written to disk - Signatures stored as ASCII-armored OpenPGP detached signatures (`application/vnd.ocm.signature.gpg`) - Credential consumer identity type: `GPG/v1alpha1` - Credential keys: `private_key_pgp`, `private_key_pgp_file`, `passphrase`, `public_key_pgp`, `public_key_pgp_file` - Includes ADR-0015 documenting the design decision - Follows RSA handler pattern exactly — no core changes required CLI integration (registering the handler as a built-in plugin) will follow in a separate PR once this module is tagged. ### Credential Config Example ```yaml # signing - type: credentials.config.ocm.software consumers: - identity: type: GPG/v1alpha1 signature: default credentials: - type: Credentials/v1 properties: private_key_pgp_file: /path/to/signing-key.asc passphrase: my-secret-passphrase ``` ## Test plan - [ ] `Unit Tests (bindings/go/gpg)` passes — round-trip sign/verify, passphrase protection, wrong passphrase, wrong public key, missing key cases - [ ] `golangci-lint (bindings/go/gpg)` passes - [ ] `Spellcheck` passes (wordlist updated for new prose words) - [ ] `DCO` passes (Signed-off-by present) - [ ] `reuse` passes (no new files without license headers — Go files inherit module license) --------- Signed-off-by: Jakob Möller <contact@jakob-moeller.com> Co-authored-by: Matthias Bruns <github@matthiasbruns.com> 9cd4df8
Registers the GPG/OpenPGP signing handler (from bindings/go/gpg, merged in open-component-model#2560) as a built-in CLI plugin alongside RSA and Sigstore. - cli/internal/plugin/builtin/gpg/register.go: new registration shim - cli/internal/plugin/builtin/builtin.go: register GPG plugin at startup - cli/go.mod: add direct dep on bindings/go/gpg - cli/integration/signing_gpg_integration_test.go: CLI-level round-trip test covering happy path, dry-run, and wrong-key verification failure - website/content/docs/tutorials/signing/gpg.md: tutorial mirroring plain.md structure, covering key generation, export, config, and sign/verify workflow Closes the CLI integration gap deferred by open-component-model#2560. On-behalf-of: @SAP <jakob.moeller@sap.com> Signed-off-by: Jakob Möller <contact@jakob-moeller.com> On-behalf-of: @SAP <jakob.moeller@sap.com>
## Summary Registers the GPG/OpenPGP signing handler (added in #2560) as a built-in CLI plugin alongside RSA and Sigstore. PR #2560 explicitly deferred CLI integration to a follow-up — this is that follow-up. - New `cli/internal/plugin/builtin/gpg/register.go` — registration shim mirroring the RSA pattern - `cli/internal/plugin/builtin/builtin.go` — registers GPG plugin at CLI startup - `cli/go.mod` — adds direct dependency on `bindings/go/gpg` - `cli/integration/signing_gpg_integration_test.go` — CLI-level round-trip integration test covering: - Happy path: sign then verify - Dry-run: verify must fail after `--dry-run`, then succeed after real sign - Wrong key: verify with a different public key must fail - `website/content/docs/tutorials/signing/gpg.md` — tutorial (weight 30, after plain.md/pem.md) covering GPG key generation, export, credential config, and sign/verify workflow ## Test plan - [ ] `Build (cli)` passes — `go build ./...` in `cli/` - [ ] `golangci-lint (cli)` passes - [ ] `Integration Tests (cli)` passes — `Test_Integration_Signing_GPG` round-trip with live OCI registry - [ ] `Spellcheck` passes - [ ] `DCO` passes --------- Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
## Summary Registers the GPG/OpenPGP signing handler (added in #2560) as a built-in CLI plugin alongside RSA and Sigstore. PR #2560 explicitly deferred CLI integration to a follow-up — this is that follow-up. - New `cli/internal/plugin/builtin/gpg/register.go` — registration shim mirroring the RSA pattern - `cli/internal/plugin/builtin/builtin.go` — registers GPG plugin at CLI startup - `cli/go.mod` — adds direct dependency on `bindings/go/gpg` - `cli/integration/signing_gpg_integration_test.go` — CLI-level round-trip integration test covering: - Happy path: sign then verify - Dry-run: verify must fail after `--dry-run`, then succeed after real sign - Wrong key: verify with a different public key must fail - `website/content/docs/tutorials/signing/gpg.md` — tutorial (weight 30, after plain.md/pem.md) covering GPG key generation, export, credential config, and sign/verify workflow ## Test plan - [ ] `Build (cli)` passes — `go build ./...` in `cli/` - [ ] `golangci-lint (cli)` passes - [ ] `Integration Tests (cli)` passes — `Test_Integration_Signing_GPG` round-trip with live OCI registry - [ ] `Spellcheck` passes - [ ] `DCO` passes --------- Signed-off-by: Jakob Möller <contact@jakob-moeller.com> f6930ce
Summary
Adds the
bindings/go/gpgmodule implementing GPG/OpenPGP signing support for OCM component versions.Closes open-component-model/ocm#1544
ocm.software/open-component-model/bindings/go/gpggithub.com/ProtonMail/go-crypto(already a transitive dep in CLI)application/vnd.ocm.signature.gpg)GPG/v1alpha1private_key_pgp,private_key_pgp_file,passphrase,public_key_pgp,public_key_pgp_fileCLI integration (registering the handler as a built-in plugin) will follow in a separate PR once this module is tagged.
Credential Config Example
Test plan
Unit Tests (bindings/go/gpg)passes — round-trip sign/verify, passphrase protection, wrong passphrase, wrong public key, missing key casesgolangci-lint (bindings/go/gpg)passesSpellcheckpasses (wordlist updated for new prose words)DCOpasses (Signed-off-by present)reusepasses (no new files without license headers — Go files inherit module license)