feat(server): add ELF signing using delivery-kit-sdk#389
Conversation
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
The e2e job b Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
…ation Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
…ocal testing) Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
There was a problem hiding this comment.
Automated code review of the ELF signing feature. 7 findings (2 MAJOR, 5 MINOR), all posted as inline comments below. No blockers. Verified correct and not flagged: the ELF-embed → PGP → TUF ordering all bind the same final bytes; temp-file perms (0600) and cleanup on every error/success path; peek/replay for non-ELF and <4-byte inputs; the interface signature change threaded through the sole caller; unsupported-arch objcopy failing as a returned error. Note: this PR adds two new external dependencies (deckhouse/delivery-kit-sdk, sigstore/sigstore) plus a large transitive bump, and touches signing-key/credential handling.
(generated by pi-pi)
| } | ||
|
|
||
| for envKey, val := range envs { | ||
| origVal, _ := os.LookupEnv(envKey) |
There was a problem hiding this comment.
MAJOR: origVal, _ := os.LookupEnv(envKey) discards the presence bool. The returned restore closure always calls os.Setenv(key, origVal), so any VAULT_* variable that was originally unset is left set to "" after signing instead of being unset. This leaks empty/stale values into the process environment across signings and to any other in-process consumer of VAULT_ADDR / VAULT_ROLE_ID / etc. Record whether each var was present and os.Unsetenv on restore when it wasn't.
(generated by pi-pi)
| } | ||
| }() | ||
|
|
||
| if _, deferErr = io.Copy(tmp, br); deferErr != nil { |
There was a problem hiding this comment.
MAJOR: for an ELF artifact this copies the entire artifact into os.TempDir() with no size cap, while publisher.mu is held for the whole StageReleaseTarget call. A large or malicious ELF can exhaust temp storage and block all publisher operations. Consider a size limit and/or a configurable spool directory, and document the disk requirement.
(generated by pi-pi)
| // configures Vault transit access only through process-global env vars | ||
| // (VAULT_ADDR, role/secret id, etc.), so concurrent signings from different | ||
| // plugin mounts would otherwise race and leak settings into each other. | ||
| var signMu sync.Mutex |
There was a problem hiding this comment.
MINOR: signMu serializes Sign, but the env vars it mutates are process-global; unrelated goroutines reading VAULT_ADDR (or the vault client used elsewhere) during a signing window can observe the mutated values. Serialization prevents signing-vs-signing races but not signing-vs-other-reader races. Prefer an explicit client-configuration API over environment variables if the SDK allows; otherwise document the constraint.
(generated by pi-pi)
There was a problem hiding this comment.
We postpone it for better days, when we adopt delivery-kit-sdk to passing vault settings like opts instead of envs
|
|
||
| passFunc := cryptoutils.SkipPassword | ||
| if opts.KeyPassword != "" { | ||
| passFunc = cryptoutils.StaticPasswordFunc([]byte(opts.KeyPassword)) |
There was a problem hiding this comment.
MINOR: a non-empty password supplied together with a hashivault:// key reference is silently ignored (as documented) but not rejected at config time. Reject it during validation so operators do not believe their password protects the remote key.
(generated by pi-pi)
| origEnvs[envKey] = origVal | ||
|
|
||
| if err := os.Setenv(envKey, val); err != nil { | ||
| panic(fmt.Errorf("failed to set env var %q: %w", envKey, err)) |
There was a problem hiding this comment.
MINOR: setVaultEnvVars panics if os.Setenv fails (same in the restore closure). Though rare, a panic on the plugin publish path crashes the process rather than failing a single publication; return an error instead.
(generated by pi-pi)
| return NewErrIncorrectTargetPath(releaseFilePath) | ||
| } | ||
|
|
||
| source, err := publisher.trySignELF(ctx, storage, releaseFilePath, data) |
There was a problem hiding this comment.
MINOR: trySignELF/StageReleaseTarget have no publisher-level tests. Missing coverage: ELF detection, non-ELF pass-through, temp cleanup on error, unsupported-arch rejection, malformed ELF, and copy failure. EM_AARCH64 is accepted but only amd64 is exercised end-to-end — add focused unit tests and ideally an arm64 signing path test.
(generated by pi-pi)
| return fmt.Errorf("%q is required", fieldNameELFSigningKey) | ||
| } | ||
|
|
||
| if strings.Contains(settings.KeyRef, "://") { |
There was a problem hiding this comment.
MINOR: strings.Contains(settings.KeyRef, "://") selects the reference branch (fine, since the base64 alphabet excludes :), but signing.go uses HasPrefix(hashivault.ReferenceScheme) — keep detection consistent. Validation also accepts trailing garbage after a certificate/intermediate PEM block and does not verify certificate/key correspondence; the sign operation would catch a mismatch later, but config-time validation could be stricter.
(generated by pi-pi)
There was a problem hiding this comment.
In this branch, we need to select all the links, including hashivault. The code below distinguishes the hashivault link from other links that are not supported.
But i changed cert/key correspondence
setVaultEnvVars restored every captured var with os.Setenv, so vars that were originally absent were left set to "" instead of being unset, polluting the process environment for other in-process consumers. Record per-key presence via os.LookupEnv and, on restore, os.Unsetenv vars that were originally absent. Make setup transactional: a mid-setup failure rolls back already-applied vars and returns a wrapped error instead of panicking, and the restore path returns joined errors that Sign surfaces alongside the signing result. Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
…config A `password` supplied alongside a `hashivault://` key reference was silently ignored, misleading operators into thinking it protected the remote key. Reject it at config-validation time instead. Addresses PR #389 review finding m4. Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
…emp-disk use and password prohibition Signed-off-by: Dmitry Mordvinov <dmitry.mordvinov@flant.com>
1 participant
No description provided.