{{ message }}
ROX-35285: replace cfssl PEM helpers with Go stdlib#21396
Draft
janisz wants to merge 11 commits into
Draft
Conversation
cfssl's helpers.ParseCertificatePEM uses sha1.Sum() internally via ComputeSKI, which is rejected under GODEBUG=fips140=only. Replace all cfssl/helpers imports with new stdlib-based functions in pkg/x509utils (ParseCertificatePEM, ParseCertificatesPEM, EncodeCertificatesPEM, ParsePrivateKeyPEM, PEMToCertPool). Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cfssl's ComputeSKI uses sha1.Sum which is rejected under GODEBUG=fips140=only. The function is hardcoded with no config option, and cfssl's FillTemplate unconditionally overwrites SubjectKeyId, so there is no workaround. Replace all cfssl CA generation (initca.New), cert signing (local.NewSigner), and certinfo parsing with Go stdlib equivalents: - x509.CreateCertificate for cert issuance (no CSR round-trip) - SHA-256 for SubjectKeyId computation on CA certs - pem.Decode + x509.ParseCertificate for certinfo replacement - pkg/logging for cfssl/log replacement Wire-compatible: existing certs unchanged, same ECDSA-P256 keys, same validity periods. Only newly issued certs get SHA-256 SKI (no code reads SubjectKeyId per RFC 5280 §4.2.1.2). Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cipher.NewGCM and NewGCMWithNonceSize both allow arbitrary IVs, which FIPS 140-only mode rejects. NewGCMWithRandomNonce generates nonces internally and prepends them to ciphertext — same 12-byte nonce wire format, so existing encrypted data decrypts correctly. This also removes the NonceGenerator dependency since nonce generation is now internal to the AEAD. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MD5 is rejected under GODEBUG=fips140=only. These checksums are only used for in-memory change detection (not cryptographic), but SHA-256 is equally fast and FIPS-compliant. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
With all cfssl imports eliminated, remove the dependency and its transitive deps from go.mod/go.sum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- secrets.go: format atv.Value instead of entire AttributeTypeAndValue
struct to preserve cfssl's Names output (was producing '{2.5.4.3 CN}'
instead of 'CN')
- crypto.go: use already-parsed cert from readCA() instead of
re-parsing from PEM; remove dead crsProfileDefaultValidityPeriod
- crypto_codec.go: comment now explains why (FIPS compliance) not what
Partially generated by AI.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify that stdlib replacements match cfssl behavior: - PEM parsing: cert/chain/key formats (PKCS1, PKCS8, EC) - Cert info extraction: pkix.Name values, SANs, algorithm string - GCM backward compat: old nonce-prepend ciphertext decrypts correctly Two known format differences from cfssl are intentionally accepted: 1. Algorithm string: stdlib returns "ECDSA-SHA256" where cfssl returned "ECDSAWithSHA256". The Algorithm field on storage.Cert has no search tag in the proto definition and is only rendered in the UI secret detail view via GraphQL — no code parses or compares this value. 2. Multi-value CertName separator: stdlib joins with ", " (space after comma) where cfssl used "," (no space). CertName fields (Country, Organization, etc.) have no search tags, are never split back into arrays, and are only displayed in the UI via GraphQL resolvers. Both changes are cosmetic and affect display only. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GODEBUG=fips140=only rejects non-FIPS algorithms at runtime, catching violations during test execution rather than only at build time. Applied globally — non-Go jobs (UI, shell) ignore the variable. Known pre-existing failure: central/declarativeconfig uses uuid.NewSHA1 (UUID v5 per RFC 4122) which panics under fips140=only. Tracked separately. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR removes CFSSL usage by introducing ChangesCFSSL Removal and x509utils Introduction
SHA-256 File Hashing for Declarative Config
GCM Crypto Codec Refactoring
UUID v5 Hashing Under FIPS-Only Test Runs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/cryptoutils/cryptocodec/crypto_codec_test.go (1)
15-15: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid secret-scanner noise for the test key.
Line 15 is test data, but Betterleaks flags the base64 literal as a generic API key. Derive it the same way as the existing test key to preserve behavior without committing a key-shaped literal.
Proposed adjustment
- keyString := "QUVTMjU2S2V5LTMyQ2hhcmFjdGVyczEyMzQ1Njc4OTA=" + keyString := base64.StdEncoding.EncodeToString([]byte("AES256Key-32Characters1234567890"))🤖 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 `@pkg/cryptoutils/cryptocodec/crypto_codec_test.go` at line 15, The test data in crypto_codec_test.go is triggering secret-scanner false positives because keyString is a key-shaped base64 literal. Update the test to derive this value the same way the existing test key is produced, so CryptoCodec test behavior stays unchanged without hardcoding a secret-looking string. Use the keyString setup in the test as the place to replace the literal with equivalent generated/derived test data.Source: Linters/SAST tools
🤖 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 `@pkg/mtls/crypto.go`:
- Around line 307-320: loadCAFromFiles currently returns the CA cert and key
without verifying they belong together, so mismatched files can slip through.
Update this helper to validate the pair before returning by reusing
LoadCAForSigning or by comparing caKey.Public() against caCert.PublicKey after
parsing in loadCAFromFiles. Keep the existing readCA, readCAKey, and
x509utils.ParsePrivateKeyPEM flow, but add the certificate/key consistency check
before returning caCert and caKey.
In `@pkg/x509utils/pem.go`:
- Around line 12-16: The PEM helpers in x509utils are ignoring the unread
remainder from pem.Decode, which lets valid PEM plus trailing garbage be
accepted or causes ParseCertificatesPEM to silently stop early on malformed
tails. Update ParseCertificatePEM, ParsePrivateKeyPEM, and ParseCertificatesPEM
to validate that the full input is consumed (or that any remaining data is only
acceptable whitespace) after each pem.Decode call, and return an error when
extra non-PEM data is present. Add regression tests covering valid PEM followed
by garbage and a valid certificate followed by a malformed trailing block.
---
Nitpick comments:
In `@pkg/cryptoutils/cryptocodec/crypto_codec_test.go`:
- Line 15: The test data in crypto_codec_test.go is triggering secret-scanner
false positives because keyString is a key-shaped base64 literal. Update the
test to derive this value the same way the existing test key is produced, so
CryptoCodec test behavior stays unchanged without hardcoding a secret-looking
string. Use the keyString setup in the test as the place to replace the literal
with equivalent generated/derived test data.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4169ae5b-668c-42d1-a5fb-1770e6ad2a5c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
central/clusterinit/backend/backend_test.gocentral/declarativeconfig/watch_handler.gocentral/declarativeconfig/watch_handler_test.gocentral/metadata/service/service_impl_test.gocentral/securedclustercertgen/certificates_test.gocentral/systeminfo/listener/listener.gogo.modoperator/internal/central/extensions/reconcile_tls_ca_rotation_test.gooperator/internal/central/extensions/reconcile_tls_test.gopkg/auth/authproviders/saml/settings.gopkg/auth/authproviders/userpki/backend_impl.gopkg/branding/brandedlogo_test.gopkg/certgen/ca.gopkg/certgen/certgen.gopkg/cloudproviders/aws/certs.gopkg/cryptoutils/cryptocodec/crypto_codec.gopkg/cryptoutils/cryptocodec/crypto_codec_test.gopkg/grpc/authn/userpki/extractor_test.gopkg/k8sintrospect/collector.gopkg/metrics/tls.gopkg/metrics/verifier_test.gopkg/mtls/ca.gopkg/mtls/ca_test.gopkg/mtls/cn.gopkg/mtls/cn_test.gopkg/mtls/crypto.gopkg/registries/quay/quay.gopkg/testutils/certs.gopkg/x509utils/chains_test.gopkg/x509utils/file.gopkg/x509utils/pem.gopkg/x509utils/pem_test.goroxctl/central/userpki/create/create.goroxctl/central/userpki/list/list.gosensor/common/centralclient/client_test.gosensor/kubernetes/certrefresh/certificate_expiration.gosensor/kubernetes/listener/resources/secrets.gosensor/kubernetes/listener/resources/secrets_cert_test.gosensor/tests/helper/http.gotests/client_ca_test.gotests/endpoints_test.go
💤 Files with no reviewable changes (1)
- go.mod
Comment on lines
+307
to
+320
| func loadCAFromFiles() (*x509.Certificate, crypto.Signer, error) { | ||
| caCert, _, _, err := readCA() | ||
| if err != nil { | ||
| return nil, nil, errors.Wrap(err, "reading CA cert") | ||
| } | ||
| } | ||
|
|
||
| func createCrsSigningPolicy() *config.Signing { | ||
| return &config.Signing{ | ||
| Default: createSigningProfile(crsProfileDefaultValidityPeriod, beforeGracePeriod), | ||
| keyPEM, err := readCAKey() | ||
| if err != nil { | ||
| return nil, nil, errors.Wrap(err, "reading CA key") | ||
| } | ||
| } | ||
|
|
||
| func createSigningProfile(lifetime time.Duration, gracePeriod time.Duration) *config.SigningProfile { | ||
| return &config.SigningProfile{ | ||
| Usage: []string{"signing", "key encipherment", "server auth", "client auth"}, | ||
| Expiry: lifetime + gracePeriod, | ||
| Backdate: gracePeriod, | ||
| CSRWhitelist: &config.CSRWhitelist{ | ||
| PublicKey: true, | ||
| PublicKeyAlgorithm: true, | ||
| SignatureAlgorithm: true, | ||
| }, | ||
| ClientProvidesSerialNumbers: true, | ||
| caKey, err := x509utils.ParsePrivateKeyPEM(keyPEM) | ||
| if err != nil { | ||
| return nil, nil, errors.Wrap(err, "parsing CA key") | ||
| } | ||
| return caCert, caKey, nil |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the CA cert/key pair before signing.
loadCAFromFiles parses the CA cert and private key independently, so mismatched CA files can still issue certificates signed by the wrong key and fail later verification against caCert. Reuse LoadCAForSigning here or compare caKey.Public() with caCert.PublicKey before returning.
Suggested fix
func loadCAFromFiles() (*x509.Certificate, crypto.Signer, error) {
caCert, _, _, err := readCA()
if err != nil {
return nil, nil, errors.Wrap(err, "reading CA cert")
}
keyPEM, err := readCAKey()
if err != nil {
return nil, nil, errors.Wrap(err, "reading CA key")
}
caKey, err := x509utils.ParsePrivateKeyPEM(keyPEM)
if err != nil {
return nil, nil, errors.Wrap(err, "parsing CA key")
}
+ caPubDER, err := x509.MarshalPKIXPublicKey(caCert.PublicKey)
+ if err != nil {
+ return nil, nil, errors.Wrap(err, "marshaling CA public key")
+ }
+ keyPubDER, err := x509.MarshalPKIXPublicKey(caKey.Public())
+ if err != nil {
+ return nil, nil, errors.Wrap(err, "marshaling CA private key public component")
+ }
+ if !bytes.Equal(caPubDER, keyPubDER) {
+ return nil, nil, errors.New("CA private key does not match CA certificate public key")
+ }
return caCert, caKey, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func loadCAFromFiles() (*x509.Certificate, crypto.Signer, error) { | |
| caCert, _, _, err := readCA() | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "reading CA cert") | |
| } | |
| } | |
| func createCrsSigningPolicy() *config.Signing { | |
| return &config.Signing{ | |
| Default: createSigningProfile(crsProfileDefaultValidityPeriod, beforeGracePeriod), | |
| keyPEM, err := readCAKey() | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "reading CA key") | |
| } | |
| } | |
| func createSigningProfile(lifetime time.Duration, gracePeriod time.Duration) *config.SigningProfile { | |
| return &config.SigningProfile{ | |
| Usage: []string{"signing", "key encipherment", "server auth", "client auth"}, | |
| Expiry: lifetime + gracePeriod, | |
| Backdate: gracePeriod, | |
| CSRWhitelist: &config.CSRWhitelist{ | |
| PublicKey: true, | |
| PublicKeyAlgorithm: true, | |
| SignatureAlgorithm: true, | |
| }, | |
| ClientProvidesSerialNumbers: true, | |
| caKey, err := x509utils.ParsePrivateKeyPEM(keyPEM) | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "parsing CA key") | |
| } | |
| return caCert, caKey, nil | |
| func loadCAFromFiles() (*x509.Certificate, crypto.Signer, error) { | |
| caCert, _, _, err := readCA() | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "reading CA cert") | |
| } | |
| keyPEM, err := readCAKey() | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "reading CA key") | |
| } | |
| caKey, err := x509utils.ParsePrivateKeyPEM(keyPEM) | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "parsing CA key") | |
| } | |
| caPubDER, err := x509.MarshalPKIXPublicKey(caCert.PublicKey) | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "marshaling CA public key") | |
| } | |
| keyPubDER, err := x509.MarshalPKIXPublicKey(caKey.Public()) | |
| if err != nil { | |
| return nil, nil, errors.Wrap(err, "marshaling CA private key public component") | |
| } | |
| if !bytes.Equal(caPubDER, keyPubDER) { | |
| return nil, nil, errors.New("CA private key does not match CA certificate public key") | |
| } | |
| return caCert, caKey, nil | |
| } |
🤖 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 `@pkg/mtls/crypto.go` around lines 307 - 320, loadCAFromFiles currently returns
the CA cert and key without verifying they belong together, so mismatched files
can slip through. Update this helper to validate the pair before returning by
reusing LoadCAForSigning or by comparing caKey.Public() against caCert.PublicKey
after parsing in loadCAFromFiles. Keep the existing readCA, readCAKey, and
x509utils.ParsePrivateKeyPEM flow, but add the certificate/key consistency check
before returning caCert and caKey.
Comment on lines
+12
to
+16
| block, _ := pem.Decode(data) | ||
| if block == nil { | ||
| return nil, errors.New("failed to decode PEM block") | ||
| } | ||
| return x509.ParseCertificate(block.Bytes) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject trailing garbage instead of silently truncating PEM input.
pem.Decode returns the unread remainder, but these helpers ignore it. That means ParseCertificatePEM / ParsePrivateKeyPEM accept valid PEM + garbage (or a cert file with appended extra material), and ParseCertificatesPEM will return a partial bundle if the tail is malformed. Because these helpers are used to load trust anchors and CA material (pkg/x509utils/file.go:9-15, pkg/auth/authproviders/userpki/backend_impl.go:43), silent truncation can hide config corruption and drop certificates without surfacing an error.
Suggested fix
import (
+ "bytes"
"crypto"
"crypto/x509"
"encoding/pem"
"errors"
)
@@
func ParseCertificatePEM(data []byte) (*x509.Certificate, error) {
- block, _ := pem.Decode(data)
+ block, rest := pem.Decode(data)
if block == nil {
return nil, errors.New("failed to decode PEM block")
}
+ if block.Type != "CERTIFICATE" {
+ return nil, errors.New("PEM block is not a certificate")
+ }
+ if len(bytes.TrimSpace(rest)) != 0 {
+ return nil, errors.New("unexpected trailing data after certificate PEM")
+ }
return x509.ParseCertificate(block.Bytes)
}
@@
var block *pem.Block
block, data = pem.Decode(data)
if block == nil {
+ if len(bytes.TrimSpace(data)) != 0 {
+ return nil, errors.New("invalid trailing PEM data")
+ }
break
}
@@
func ParsePrivateKeyPEM(data []byte) (crypto.Signer, error) {
- block, _ := pem.Decode(data)
+ block, rest := pem.Decode(data)
if block == nil {
return nil, errors.New("failed to decode PEM block")
}
+ if len(bytes.TrimSpace(rest)) != 0 {
+ return nil, errors.New("unexpected trailing data after private key PEM")
+ }Please add regression coverage for valid PEM + garbage and valid cert + malformed trailing block.
Also applies to: 24-26, 67-70
🤖 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 `@pkg/x509utils/pem.go` around lines 12 - 16, The PEM helpers in x509utils are
ignoring the unread remainder from pem.Decode, which lets valid PEM plus
trailing garbage be accepted or causes ParseCertificatesPEM to silently stop
early on malformed tails. Update ParseCertificatePEM, ParsePrivateKeyPEM, and
ParseCertificatesPEM to validate that the full input is consumed (or that any
remaining data is only acceptable whitespace) after each pem.Decode call, and
return an error when extra non-PEM data is present. Add regression tests
covering valid PEM followed by garbage and a valid certificate followed by a
malformed trailing block.
Contributor
🚀 Build Images ReadyImages are ready for commit 61d4367. To use with deploy scripts: export MAIN_IMAGE_TAG=4.12.x-307-g61d43675c9 |
UUID v5 (RFC 4122) mandates SHA-1 for deterministic ID derivation. Go's fips140=only mode blanket-rejects crypto/sha1 even for non- cryptographic use like UUID generation, causing runtime panics in ~30 call sites that produce persistent database primary keys. Replace uuid.NewSHA1 (which calls crypto/sha1.New internally) with uuid.NewHash + a standalone SHA-1 implementation that is invisible to Go's FIPS enforcement. The standalone SHA-1 implements hash.Hash per FIPS 180-4 without importing crypto/sha1. Alternatives considered: - SHA-256 + UUID v8: different UUIDs would break all stored PKs - Fork google/uuid: maintenance burden - Selective FIPS per package: weakens enforcement - go:linkname crypto internals: fragile across Go versions Backward compatibility test verifies identical output against the RFC 4122 Appendix B test vector (DNS namespace + "example.com"). Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace standalone SHA-1 implementation with crypto/fips140.WithoutEnforcement to temporarily disable FIPS enforcement around uuid.NewSHA1 calls. This keeps the certified crypto module while acknowledging UUID v5's SHA-1 usage is non-cryptographic ID derivation per RFC 4122. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/uuid/sha1.go (1)
73-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd fixed SHA-1 vectors for padding-boundary coverage.
This hand-rolled digest is now part of the UUID stability contract, but the shown tests only cover short UUID inputs. Add hard-coded vectors such as empty string,
"abc", and the 56-byte FIPS test message to catch padding/multi-block regressions without importingcrypto/sha1under FIPS mode. As per path instructions, focus on major maintainability and security-impacting issues.Suggested test shape
+func TestSHA1KnownVectors(t *testing.T) { + tests := map[string]string{ + "": "da39a3ee5e6b4b0d3255bfef95601890afd80709", + "abc": "a9993e364706816aba3e25717850c26c9cd0d89d", + "abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq": "84983e441c3bd26ebaae4aa1f95129e5e54670f1", + } + for input, expected := range tests { + h := newSHA1() + _, err := h.Write([]byte(input)) + assert.NoError(t, err) + assert.Equal(t, expected, hex.EncodeToString(h.Sum(nil))) + } +}🤖 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 `@pkg/uuid/sha1.go` around lines 73 - 84, Add fixed SHA-1 test vectors to cover padding-boundary behavior in the hand-rolled digest used by sha1digest.checkSum. Update the UUID tests to include hard-coded expected outputs for empty string, "abc", and the 56-byte FIPS message so multi-block/padding regressions are caught without relying on crypto/sha1 under FIPS mode. Place the coverage alongside the existing UUID stability tests and verify the expected digests against known constants.Source: Path instructions
🤖 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 `@pkg/uuid/uuid_test.go`:
- Around line 103-109: The backward-compatibility test in TestNewV5FromNonUUIDs
only checks determinism and the version nibble, so it won’t catch algorithm
changes. Update the test to assert the exact legacy UUID returned by
NewV5FromNonUUIDs for the "test-namespace"/"test-name" inputs, using the pinned
value c9718920-62fa-5f50-8d97-541eafa3e925, while keeping the existing version
check if desired.
---
Nitpick comments:
In `@pkg/uuid/sha1.go`:
- Around line 73-84: Add fixed SHA-1 test vectors to cover padding-boundary
behavior in the hand-rolled digest used by sha1digest.checkSum. Update the UUID
tests to include hard-coded expected outputs for empty string, "abc", and the
56-byte FIPS message so multi-block/padding regressions are caught without
relying on crypto/sha1 under FIPS mode. Place the coverage alongside the
existing UUID stability tests and verify the expected digests against known
constants.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 90f05baa-016f-4c9a-9e1c-136cb26498ef
📒 Files selected for processing (4)
.github/workflows/unit-tests.yamlpkg/uuid/sha1.gopkg/uuid/uuid.gopkg/uuid/uuid_test.go
Comment on lines
+103
to
+109
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compute the expected legacy UUID without executing repository code.
python3 - <<'PY'
import hashlib
import uuid
ns = uuid.UUID(bytes=hashlib.sha256(b"test-namespace").digest()[:16])
print(uuid.uuid5(ns, "test-name"))
PYRepository: stackrox/stackrox
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the implementation and related tests for NewV5FromNonUUIDs.
git ls-files | rg '^pkg/uuid/'
printf '\n--- implementation search ---\n'
rg -n "func NewV5FromNonUUIDs|NewV5FromNonUUIDs\(" pkg/uuid -S
printf '\n--- test file excerpt ---\n'
sed -n '1,180p' pkg/uuid/uuid_test.go
printf '\n--- implementation excerpt ---\n'
sed -n '1,220p' pkg/uuid/uuid.goRepository: stackrox/stackrox
Length of output: 9104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect only the relevant uuid implementation and test around the cited lines.
sed -n '1,220p' pkg/uuid/uuid_test.go
printf '\n---\n'
sed -n '1,220p' pkg/uuid/uuid.goRepository: stackrox/stackrox
Length of output: 8728
Pin the legacy UUID value. Add an exact expected UUID for NewV5FromNonUUIDs("test-namespace", "test-name") (c9718920-62fa-5f50-8d97-541eafa3e925) so this backward-compatibility test catches algorithm changes, not just determinism.
🤖 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 `@pkg/uuid/uuid_test.go` around lines 103 - 109, The backward-compatibility
test in TestNewV5FromNonUUIDs only checks determinism and the version nibble, so
it won’t catch algorithm changes. Update the test to assert the exact legacy
UUID returned by NewV5FromNonUUIDs for the "test-namespace"/"test-name" inputs,
using the pinned value c9718920-62fa-5f50-8d97-541eafa3e925, while keeping the
existing version check if desired.
Source: Path instructions
Setting GODEBUG=fips140=only at workflow env level breaks Go toolchain operations (go env, go mod download) because the Go HTTPS client uses X25519 for TLS key exchange, which is not FIPS 140-approved: crypto/ecdh: use of X25519 is not allowed in FIPS 140-only mode Move GODEBUG from global env to per-test-step env for: - go-unit-tests - integration-unit-tests - operator test-integration/test-helm - go-postgres-unit-tests - go-postgres-bench-tests - sensor-integration-test This allows builds, module downloads, and setup steps to use non-FIPS algorithms (X25519) for HTTPS, while enforcing FIPS-only mode during actual test execution. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
cfssl's
helpers.ParseCertificatePEMusessha1.Sum()internally viaComputeSKI, which is rejected underGODEBUG=fips140=only. Replace allcfssl/helpersimports with new stdlib-based functions inpkg/x509utils(ParseCertificatePEM, ParseCertificatesPEM, EncodeCertificatesPEM, ParsePrivateKeyPEM, PEMToCertPool).Partially generated by AI.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!