Repository navigation
feat: emit CycloneDX 1.7 CBOM output (#505) - #543
sachin9058 wants to merge 1 commit into
Conversation
|
Hi @san-zrl @n1ckl0sk0rtge @fynnth I have upgraded the CycloneDX from 1.6 to 1.7. Please take a look on this PR. Any improvements and suggestions will be highly appreciated. Thanks !!! |
1c0ba3a to
4a8af65
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical schema, deduplication, and model compatibility issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Upgrades CBOM output and documentation from CycloneDX 1.6 to 1.7.
Changes:
- Adds 1.7 algorithm, protocol, and certificate property models.
- Updates curve, algorithm-family, key-wrap, and protocol relationship mappings.
- Updates README examples and output tests.
File summaries
| File | Review summary |
|---|---|
README.md |
Updates documented schema version. |
output/src/test/java/com/ibm/output/cyclonedx/TestBase.java |
Moderate (3 votes): tests still generate with VERSION_16 and do not validate 1.7 serialization. |
output/src/test/java/com/ibm/output/cyclonedx/ProtocolTest.java |
Moderate (2 votes; 1 vote): relationship assertions are conditional and do not verify required 1.7 types and entries. |
output/src/test/java/com/ibm/output/cyclonedx/AlgorithmTest.java |
Updates curve assertions. |
output/src/main/java/com/ibm/output/cyclondx/CBOMOutputFile.java |
Critical (3 votes): bom-ref keyed lookup bypasses name-based deduplication, causing duplicate protocol components. |
output/src/main/java/com/ibm/output/cyclondx/builder/Utils.java |
Adjusts predicate nullability. |
output/src/main/java/com/ibm/output/cyclondx/builder/ProtocolProperties17.java |
Moderate (2 votes): related-assets return type is incompatible with the CycloneDX 1.7 model. |
output/src/main/java/com/ibm/output/cyclondx/builder/ProtocolComponentBuilder.java |
Adds related cryptographic asset support. |
output/src/main/java/com/ibm/output/cyclondx/builder/IProtocolComponentBuilder.java |
Moderate (1 vote): API accepts legacy material-property objects instead of 1.7 related-asset types. |
output/src/main/java/com/ibm/output/cyclondx/builder/IAlgorithmComponentBuilder.java |
Adds algorithm-family builder support. |
output/src/main/java/com/ibm/output/cyclondx/builder/CertificateProperties17.java |
Critical (1 vote): fingerprint and certificate extensions use schema-invalid types. Moderate (1 vote): deprecated inherited fields remain serializable. |
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmProperties17.java |
Defines 1.7 algorithm properties. |
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmComponentBuilder.java |
Critical (3 votes): secp256k1 is emitted with an invalid 1.7 curve identifier. |
Review details
Suppressed comments (12)
output/src/main/java/com/ibm/output/cyclondx/CBOMOutputFile.java:350
BomJsonGeneratoris still constructed with theschemaconstant set toVersion.VERSION_16. OverwritingBom.specVersionhere changes only the header string; it does not switch the generator's version-aware serialization/filtering, so the file can advertise 1.7 while being serialized as 1.6. Configure the generator withVersion.VERSION_17and remove this reflection workaround.
Field specVersionField = Bom.class.getDeclaredField("specVersion");
specVersionField.setAccessible(true);
specVersionField.set(bom, "1.7");
output/src/main/java/com/ibm/output/cyclondx/CBOMOutputFile.java:210
- A 1.7
relatedCryptographicAssetsitem is a relationship containing aref, not aRelatedCryptoMaterialPropertiesobject. That class serializes this value asid(andRelatedCryptoMaterialType.OTHERasother), producing entries like{type: "other", id: ...}that do not validate against the 1.7 schema or the documented output. Use a relationship DTO withsetRef(ref)and the 1.7 relationship type.
List<RelatedCryptoMaterialProperties> relatedAssets = new ArrayList<>();
for (String ref : cryptoRefs) {
RelatedCryptoMaterialProperties rcm = new RelatedCryptoMaterialProperties();
rcm.setId(ref);
rcm.setType(RelatedCryptoMaterialType.OTHER);
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmComponentBuilder.java:278
- This branch recognizes
KeyWrapbut assigns the legacyotherprimitive, so key-wrap algorithms still do not emit the new 1.7key-wrapvalue. The current 12.2.0Primitivemodel has no key-wrap constant, so this needs a 1.7-compatible primitive representation rather than a fallback toOTHER.
} else if (primitive.is(KeyWrap.class)) {
primitives = Primitive.OTHER;
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmComponentBuilder.java:342
- The fallback writes the original mapper name into the 1.7
ellipticCurvefield whenever a curve is absent from this small table. For example, the mapper hassect163k1and othersect*curves, so those values remain unnamespaced and are not members of the 1.7 enum. Do not emit the free-text fallback; either map every supported curve or omit unknown values.
String namespaced =
CURVE_TO_NAMESPACED.getOrDefault(
ellipticCurve.asString(), ellipticCurve.asString());
this.algorithmProperties.setEllipticCurve(namespaced);
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmComponentBuilder.java:109
- These mappings use non-registry values and also conflate distinct curves: the 1.7 identifiers are
other/Ed25519,other/Ed448,other/Curve25519, andother/Curve448. As written, Ed25519/Ed448 and X25519/X448 detections emit invalidx25519/x448values and lose the Ed-vs-Montgomery distinction.
CURVE_TO_NAMESPACED.put("Edwards25519", "x25519");
CURVE_TO_NAMESPACED.put("Edwards448", "x448");
CURVE_TO_NAMESPACED.put("Curve25519", "x25519");
CURVE_TO_NAMESPACED.put("Curve448", "x448");
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmComponentBuilder.java:488
algorithm.asString()includes variants and parameters, so this exact lookup missesalgorithmFamilyfor most modeled algorithms, includingAES-256-CBC,RSA-OAEP,ECDSA-secp256r1,SHA-256, andML-KEM-768. Derive the family from the base algorithm/model or normalize the variant before lookup; otherwise the new field is absent for the common outputs described by the PR.
if (algorithm != null) {
String family = ALGORITHM_TO_FAMILY.get(algorithm.asString());
if (family != null) {
this.algorithmProperties.setAlgorithmFamily(family);
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmComponentBuilder.java:119
- The values
SHA,SHA2, andSHA3are not registry enum values (SHA-1,SHA-2, andSHA-3); when these entries match, they will produce invalid BOMs. Use the registry spellings.
ALGORITHM_TO_FAMILY.put("SHA", "SHA");
ALGORITHM_TO_FAMILY.put("SHA2", "SHA2");
ALGORITHM_TO_FAMILY.put("SHA3", "SHA3");
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmComponentBuilder.java:113
- Generic
RSAis not a CycloneDX 1.7algorithmFamiliesEnumvalue; this entry makes the existing base RSA output emitalgorithmFamily: "RSA", which fails schema validation. Omit the family for generic RSA or map a concrete RSA variant to a registry family.
ALGORITHM_TO_FAMILY.put("AES", "AES");
ALGORITHM_TO_FAMILY.put("RSA", "RSA");
ALGORITHM_TO_FAMILY.put("DSA", "DSA");
output/src/main/java/com/ibm/output/cyclondx/builder/CertificateProperties17.java:32
certificateExtensionsis a structured 1.7 property, but this class serializes it as an array of strings. If these properties are wired into a certificate component, Jackson will emit the wrong shape and the CBOM will fail schema validation; use a model for the certificate-extension objects instead ofList<String>.
private List<String> certificateExtensions;
output/src/main/java/com/ibm/output/cyclondx/builder/CertificateProperties17.java:38
- This class only ignores
certificateExtension; it inherits serializablesignatureAlgorithmRefandsubjectPublicKeyReffrom the coreCertificateProperties. If this new 1.7 type is used, it still emits two deprecated fields contrary to the migration; explicitly ignore or remove both inherited accessors.
@JsonIgnore
@Override
public String getCertificateExtension() {
return null;
}
output/src/main/java/com/ibm/output/cyclondx/builder/IProtocolComponentBuilder.java:47
- This new builder API accepts old material-property objects, so callers cannot provide the CycloneDX 1.7
{type, ref}relationship entries required byrelatedCryptographicAssets. Change the parameter and implementation to use a 1.7-compatible related-asset type.
IProtocolComponentBuilder relatedCryptographicAssets(
@Nullable List<RelatedCryptoMaterialProperties> assets);
output/src/test/java/com/ibm/output/cyclonedx/ProtocolTest.java:278
- This second
instanceofguard also skips the new assertion for a legacy property object, and checking only the count does not verify that the two dependency refs are emitted correctly. Make the 1.7 type and exact{type, ref}entries mandatory.
if (protocolProperties
instanceof
com.ibm.output.cyclondx.builder.ProtocolProperties17 p17) {
assertThat(p17.getRelatedCryptographicAssets()).hasSize(2);
}
- Files reviewed: 13/13 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
3da1d2c to
1948e04
Compare
san-zrl
left a comment
There was a problem hiding this comment.
Hi @sachin9058 - Thanks for this PR. This direction of work is most welcome. However, I don't think this PR is ready to be merged yet. Two general comments:
-
You seem to implement 1.7 support on top of the 1.6 model. You should update the cyclonedx-core-java dependency to 13.2.0. Then you don't have to extend the model XXXProperties to XXXProperties17 anymore.
-
Please address ALL Copilot comments and add an explanation if you dismiss them.
Looking forward to the next version.
c081213 to
3288b5f
Compare
3288b5f to
677add6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings affect CycloneDX 1.7 validity, relationship output, and regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
output/src/main/java/com/ibm/output/cyclondx/CBOMOutputFile.java:208
addComponentAndDependenciesdeduplicates protocols by name and recurses using the stored component's bom-ref. On a later protocol with the same name,protocol.getBomRef()is not the dependency key, so this condition skipsrelatedCryptographicAssetsfor that occurrence (and any newly discovered dependencies). Look up the stored component's bom-ref instead.
if (!cryptoRefs.isEmpty()
&& protocol.getCryptoProperties() != null
&& protocol.getCryptoProperties().getProtocolProperties()
instanceof ProtocolProperties) {
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmComponentBuilder.java:343
- When a curve is not in the map, this fallback writes its free-text model name into
ellipticCurve. That field is a 1.7 registry enum, so an unrecognized value such asBrainpoolp384r1makes the generated CBOM fail schema validation; omit the field or handle the unsupported curve explicitly instead of falling back to raw text.
String namespaced =
CURVE_TO_NAMESPACED.getOrDefault(
ellipticCurve.asString(), ellipticCurve.asString());
this.algorithmProperties.setEllipticCurve(namespaced);
output/src/main/java/com/ibm/output/cyclondx/builder/AlgorithmComponentBuilder.java:120
- The SHA family values here do not match the 1.7 registry tokens (
SHA-1,SHA-2, andSHA-3). When these entries are selected, the emittedalgorithmFamilywill fail schema validation.
ALGORITHM_TO_FAMILY.put("SHA", "SHA");
ALGORITHM_TO_FAMILY.put("SHA2", "SHA2");
ALGORITHM_TO_FAMILY.put("SHA3", "SHA3");
- Files reviewed: 15/15 changed files
- Comments generated: 7
- Review effort level: Lite
182195c to
edfcbb3
Compare
|
@san-zrl Hi Andreas i have made some changes and reviewed and implemented the copilot suggestions. It would be great if u will take a look on this and help me out wheather i am in correct direction or not? Thanks !!! |
san-zrl
left a comment
There was a problem hiding this comment.
Hi @sachin9058 - Thanks for the update. Now we are getting into the details:
- CURVE_TO_NAMESPACED maps only a handful of curves. The rest is used as is. What about all the other curves in https://cyclonedx.org/docs/1.7/json/#components_items_cryptoProperties_algorithmProperties_ellipticCurve?
- Our goal is to produce schema-compliant CBOM 1 .7 json representations. You may have noticed that there is a discrepancy between the standard https://cyclonedx.org/docs/1.7/json and the model library. The standard says for the ellipticCurve property: "If specified, this value must be one of the enumeration of valid elliptic curves identifiers defined in the cryptography-defs.schema.json subschema." The library chose to implement ellipticCurve as a String. It should not be possible to emit invalid values for an enum value.
- https://cyclonedx.org/docs/1.7/json/#components_items_cryptoProperties_algorithmProperties_algorithmFamily provides way more families than you map in ALGORITHM_TO_FAMILY. Thus the mapping will fail in many cases and your code won't produce an algorithmFamily value.
- Your CycloneDx17JsonGenerator only ties in the mix-ins which only suppresses the generation of the deprecated properties. I would drop this. The library will evolve in the future anyway and I don't want to have to add special serializers every time.
- Issue #505 lists acceptance criteria for this PR. Would be good if you could confirm they are met.
f356285 to
88f4790
Compare
|
Hi @san-zrl ,
Concerns:
|
There was a problem hiding this comment.
PR-543: CycloneDX 1.6 → 1.7 CBOM Upgrade
| Verdict | Count |
|---|---|
| ❌ False / Broken Claims | 5 |
| 4 | |
| ✅ Verified Correct | 10 |
1. Core Architecture Claims
1.1 specVersion: "1.7" emitted — ✅ CORRECT
1.2 Deprecated fields removed via MixIn annotations — ✅ CORRECT
All three MixIns correctly suppress the right fields :
AlgorithmPropertiesMixIn:@JsonIgnoreProperties({"curve"})—getCurve()confirmed in library ✓ProtocolPropertiesMixIn:@JsonIgnoreProperties({"cryptoRefArray"})—getCryptoRefArray()confirmed ✓CertificatePropertiesMixIn:@JsonIgnoreProperties({"certificateExtension", "signatureAlgorithmRef", "subjectPublicKeyRef"})— all three confirmed ✓
1.3 relatedCryptographicAssets type set to "algorithm" — ✅ CORRECT
rcm.setType("algorithm") at CBOMOutputFile.java:215. Tests assert containsExactlyInAnyOrder("algorithm", "algorithm", "algorithm"). Correct per 1.7 schema.
1.4 No XXXProperties17 subclasses; MixIns used instead — ✅ CORRECT
Confirmed. No subclass approach exists in the codebase. Only MixIns and base library classes are used.
2. New 1.7 Fields Emitted
2.1 algorithmFamily — ❌ BROKEN: 7 dead map keys
The lookup in AlgorithmComponentBuilder.build() is:
ALGORITHM_TO_FAMILY.get(((Algorithm) algorithm).getName())Seven map keys will never match because the key strings do not equal what getName() actually returns:
| Map Key (wrong) | Actual getName() |
Desired algorithmFamily |
|---|---|---|
"EDDSA" |
"EdDSA" |
EdDSA |
"Camellia" |
"CAMELLIA" |
CAMELLIA |
"SCrypt" |
"scrypt" |
scrypt |
"SHA" |
"SHA-1" |
SHA-1 |
"SHA2" |
"SHA-256", "SHA-384", etc. |
SHA-2 |
"MLKEM" |
"ML-KEM" |
ML-KEM |
"MLDSA" |
"ML-DSA" |
ML-DSA |
ML-KEM and ML-DSA silently emitting no algorithmFamily is particularly significant for PQC compliance.
Two additional entries are permanently dead (no matching class in the mapper model):
"ChaCha"— no standaloneChaChaclass; onlyChaCha20withgetName()="ChaCha20"."CMEA"— no CMEA algorithm class exists in the mapper model.
SHA-3 only partially works: bare new SHA3(detectionLocation) → getName()="SHA3" matches the key; new SHA3(256, detectionLocation) → getName()="SHA3-256" does not.
There is also a harmless duplicate: ALGORITHM_TO_FAMILY.put("SEED", "SEED") appears twice.
No tests assert algorithmFamily correctness, so all these failures are silent.
2.2 RSA family always maps to "RSAES-PKCS1" — ⚠️ MISLEADING
RSA.getName()→"RSA"→ maps to"RSAES-PKCS1". Correct for plain RSA.RSAssaPSS.getName()→"RSA-PSS"→ no match → noalgorithmFamilyemitted for RSA-PSS.- The PR states "RSA-PSS can't be distinguished by name alone" — this is false.
RSAssaPSSalready has a distinctgetName()="RSA-PSS"and could be mapped toRSASSA-PSSdirectly.
2.3 ellipticCurve namespaced registry identifiers — ⚠️ PARTIAL: 10 curves missing
The 13 mapped curves are correct. However 10 curves defined in the mapper model have no entry in CURVE_TO_NAMESPACED and will fall back to the raw name, which is invalid per the 1.7 schema registry:
Curve (asString()) |
Required registry ID | Emitted (fallback) |
|---|---|---|
sect163k1 |
secg/sect163k1 |
sect163k1 ✗ |
sect163r2 |
secg/sect163r2 |
sect163r2 ✗ |
sect233k1 |
secg/sect233k1 |
sect233k1 ✗ |
sect233r1 |
secg/sect233r1 |
sect233r1 ✗ |
sect283k1 |
secg/sect283k1 |
sect283k1 ✗ |
sect283r1 |
secg/sect283r1 |
sect283r1 ✗ |
sect409k1 |
secg/sect409k1 |
sect409k1 ✗ |
sect409r1 |
secg/sect409r1 |
sect409r1 ✗ |
sect571k1 |
secg/sect571k1 |
sect571k1 ✗ |
sect571r1 |
secg/sect571r1 |
sect571r1 ✗ |
Note: The PR description example shows
"brainpool/P256R1"but the actual code correctly emits"brainpool/brainpoolP256r1", which matches the schema.
2.4 relatedCryptographicAssets replaces cryptoRefArray — ✅ CORRECT
ProtocolProperties.setRelatedCryptographicAssets() is called. getCryptoRefArray() is suppressed via MixIn. Verified by test protocolWithCipherSuite asserting both relatedAssets.hasSize(3) and getCryptoRefArray() == null.
2.5 certificateFileExtension, serialNumber, fingerprint added — ❌ FALSE
These three fields are claimed to be "Added to certificate properties", but cyclonedx-core-java 13.2.0's CertificateProperties does not contain any of them. Confirmed via javap:
public java.lang.String getCertificateExtension(); // deprecated, still present
public java.lang.String getSignatureAlgorithmRef(); // deprecated, still present
public java.lang.String getSubjectPublicKeyRef(); // deprecated, still present
// NO getCertificateFileExtension(), getSerialNumber(), getFingerprint()
There is no code anywhere in the PR that sets these fields on any object. This claim is false for this library version.
3. Curve Mapping — Full Correctness Check
All 23 curve classes in the mapper model verified against cryptography-defs.schema.json:
Curve (asString()) |
Emitted | Correct? |
|---|---|---|
secp256r1 |
nist/P-256 |
✅ |
secp384r1 |
nist/P-384 |
✅ |
secp521r1 |
nist/P-521 |
✅ |
secp256k1 |
secg/secp256k1 |
✅ |
secp224r1 |
nist/P-224 |
✅ |
secp192r1 |
nist/P-192 |
✅ |
Brainpoolp256r1 |
brainpool/brainpoolP256r1 |
✅ |
Brainpoolp384r1 |
brainpool/brainpoolP384r1 |
✅ |
Brainpoolp512r1 |
brainpool/brainpoolP512r1 |
✅ |
Edwards25519 |
other/Ed25519 |
✅ |
Edwards448 |
other/Ed448 |
✅ |
Curve25519 |
other/Curve25519 |
✅ |
Curve448 |
other/Curve448 |
✅ |
sect163k1 |
sect163k1 (should be secg/sect163k1) |
❌ |
sect163r2 |
sect163r2 (should be secg/sect163r2) |
❌ |
sect233k1 |
sect233k1 (should be secg/sect233k1) |
❌ |
sect233r1 |
sect233r1 (should be secg/sect233r1) |
❌ |
sect283k1 |
sect283k1 (should be secg/sect283k1) |
❌ |
sect283r1 |
sect283r1 (should be secg/sect283r1) |
❌ |
sect409k1 |
sect409k1 (should be secg/sect409k1) |
❌ |
sect409r1 |
sect409r1 (should be secg/sect409r1) |
❌ |
sect571k1 |
sect571k1 (should be secg/sect571k1) |
❌ |
sect571r1 |
sect571r1 (should be secg/sect571r1) |
❌ |
4. Algorithm Family Mapping — Working Keys
These getName() values correctly resolve to valid CycloneDX 1.7 schema families at runtime:
AES→AES, 3DES→3DES, DES→DES, ARIA→ARIA, CAMELLIA→CAMELLIA, ChaCha20→ChaCha20,
DSA→DSA, ECDH→ECDH, ECDSA→ECDSA, ECIES→ECIES, HKDF→HKDF, HMAC→HMAC,
PBKDF2→PBKDF2, Poly1305→Poly1305, RC2→RC2, RC4→RC4, RC5→RC5, RC6→RC6,
RSA→RSAES-PKCS1*, SEED→SEED, SLH-DSA→SLH-DSA, SHA3 (bare only)→SHA-3,
Salsa20→Salsa20, Serpent→Serpent, Twofish→Twofish, CMAC→CMAC, IDEA→IDEA,
Blowfish→Blowfish, CAST5→CAST5, CAST6→CAST6, Argon2→Argon2,
SM2→SM2, SM3→SM3, SM4→SM4, scrypt→scrypt*
* RSA maps correctly only for plain RSA, not RSA-PSS or RSA-OAEP. scrypt value is correct but the key "SCrypt" never matches getName()="scrypt".
5. Test Coverage Gaps
Tests pass but provide false confidence about the 1.7-specific fields:
- No test asserts
getAlgorithmFamily()is set to any expected value. - Only 2 tests check
getEllipticCurve()(secp256r1andsecp384r1only). - No test verifies
getCurve()is absent from serialized JSON output (MixIn effectiveness). - No test verifies
getCryptoRefArray()is absent from serialized JSON output. - No test covers any
sect*curve — the 10 missing mappings are entirely untested. - No test verifies
algorithmFamilyfor SHA-1, SHA-2, EdDSA, ML-KEM, or ML-DSA.
6. PR Claims vs Reality
| PR Claim | Verdict |
|---|---|
specVersion: "1.7" emitted |
✅ True |
algorithmFamily added to all algorithms |
❌ False — 7 keys never match; EdDSA, CAMELLIA, scrypt, SHA-1, SHA-2, ML-KEM, ML-DSA silently produce no family |
ellipticCurve namespaced registry IDs |
sect* curves emit invalid unnamespaced fallbacks |
relatedCryptographicAssets replaces cryptoRefArray |
✅ True |
certificateFileExtension, serialNumber, fingerprint added |
❌ False — not in library 13.2.0; no code sets them anywhere |
curve, cryptoRefArray, deprecated cert fields removed from output |
✅ True (via MixIns) |
| "246 curves and 93 algorithm families covered" | ❌ False — 13 curves mapped; ~34 algorithm families resolve at runtime |
| All tests pass | ✅ True (but 1.7-field test coverage is very thin) |
algorithmFamily() builder method used for family lookup |
CBOMOutputFile; only the build()-time map lookup runs |
RelatedCryptographicAsset.type = "algorithm" |
✅ True |
88f4790 to
d2b64b2
Compare
|
@san-zrl All issues from the review have been addressed:
|
d2b64b2 to
1e9f1bf
Compare
san-zrl
left a comment
There was a problem hiding this comment.
Hi @sachin9058 - The PR is getting better but there are still remaining issues that must be resolved before we can merge
-
"RSA-KEM" → "RSA-KEM" is not a valid algorithmFamily value. The CycloneDX 1.7 cryptography-defs.schema.json does not include "RSA-KEM" in algorithmFamiliesEnum. RSAKEM.getName() returns "RSA-KEM", which hits this entry and emits an invalid family value.
-
27 curve names as dead keys in ALGORITHM_TO_FAMILY. The new map adds entries
"secp192r1" → "ECDSA"
"secp224r1" → "ECDSA"
"secp256r1" → "ECDSA"
"secp256k1" → "ECDSA"
"secp384r1" → "ECDSA"
"secp521r1" → "ECDSA"
"Brainpoolp256r1" → "ECDSA"
"Brainpoolp384r1" → "ECDSA"
"Brainpoolp512r1" → "ECDSA"
"Edwards25519" → "EdDSA"
"Edwards448" → "EdDSA"
"Curve25519" → "ECDH"
"Curve448" → "ECDH"
"sect163k1" → "ECDSA"
"sect163r2" → "ECDSA"
"sect233k1" → "ECDSA"
"sect233r1" → "ECDSA"
"sect283k1" → "ECDSA"
"sect283r1" → "ECDSA"
"sect409k1" → "ECDSA"
"sect409r1" → "ECDSA"
"sect571k1" → "ECDSA"
"sect571r1" → "ECDSA"
"X25519" → "ECDH"
"X448" → "ECDH"
"Ed25519" → "EdDSA"
"Ed448" → "EdDSA"
These keys can never be reached because build() calls ALGORITHM_TO_FAMILY.get(((Algorithm) algorithm).getName()), and curve objects travel through curve() → setEllipticCurve(), not through algorithm(). The algorithm getName() for an ECDSA object is "ECDSA", not "secp256r1". These 27 entries are harmless dead weight but reflect a conceptual confusion about the data flow.
-
SHA-2 gaps: "SHA-224", "SHA-512/224", "SHA-512/256" not in map. SHA2(224) → getName() = "SHA-224". The truncated SHA-512 variants produce names like "SHA-512/224". None are in the map, so these emit no algorithmFamily. Minor but incomplete.
-
"Aria" dead key. Aria.getName() returns "ARIA", which correctly hits the "ARIA" key. The additional "Aria" key is a dead entry (harmless).
-
Test coverage improved. Still no coverage for: algorithmFamily on SHA-1/SHA-2/SHA-3, EdDSA, ML-KEM, ML-DSA, scrypt,
|
@san-zrl All 5 issues have been addressed:
Please let me know if i am going in write direction... |
san-zrl
left a comment
There was a problem hiding this comment.
Hi @sachin9058 - Good progress. Please correct the two remaining bugs. Tx for your perseverance.
PR-543 Evaluation — v3
What changed from v2
ALGORITHM_TO_FAMILY: 27 dead curve-name keys removed,SHA-224/SHA-512/224/SHA-512/256gaps filled,RSA-KEMentry removedbuild()gains anEC-prefix fallback forEllipticCurveAlgorithm- New tests added for SHA-1, SHA-2, SHA-3, EdDSA, ML-KEM, ML-DSA, scrypt, RSA-KEM (expects null),
rsaKEMNoFamily
✅ Fixed from v2
SHA-224,SHA-512/224,SHA-512/256now mapped toSHA-2RSA-KEM → "RSA-KEM"(invalid schema value) removed —rsaKEMNoFamilytest explicitly assertsgetAlgorithmFamily() == null, acknowledging the library doesn't support this value- 27 dead curve-name keys removed from
ALGORITHM_TO_FAMILY - Tests now cover SHA-1, SHA-2, SHA-3, EdDSA, ML-KEM, ML-DSA, scrypt
❌ New bugs introduced in v3
1. EC- fallback always produces "ECDSA" — wrong for Montgomery and Edwards curves
if (family == null && name.startsWith("EC-")) {
family = ALGORITHM_TO_FAMILY.get(name.substring(3)); // e.g. "Curve25519" — not in map → null
}
if (family == null && name.startsWith("EC-")) {
family = "ECDSA"; // hardcoded fallback — always reached
}The first EC- block strips the prefix and looks up the curve name (e.g. "Curve25519", "Edwards25519") — but those keys were removed from ALGORITHM_TO_FAMILY in this version, so the lookup always returns null. The second block then hardcodes "ECDSA", meaning every EllipticCurveAlgorithm gets "ECDSA" regardless of curve:
| Curve | Emitted family | Correct family |
|---|---|---|
EC-secp256r1 |
ECDSA |
✅ |
EC-sect163k1 |
ECDSA |
✅ |
EC-Curve25519 |
ECDSA |
❌ should be ECDH |
EC-Curve448 |
ECDSA |
❌ should be ECDH |
EC-Edwards25519 |
ECDSA |
❌ should be EdDSA |
EC-Edwards448 |
ECDSA |
❌ should be EdDSA |
The first EC- block (lines 520–522) is entirely dead code — it can never produce a non-null result because the curve-name keys it would look up no longer exist in the map.
2. SHA3 (bare, no digest size) still has no family
SHA3.getName() = "SHA3" — not in the map. SHA3(256) = "SHA3-256" is in the map. The test only covers new SHA3(256, ...). A bare new SHA3(detectionLocation) emits no algorithmFamily. Minor, as bare SHA3 is uncommon in practice.
Summary table
| Item | v1 | v2 | v3 |
|---|---|---|---|
| All 23 curves mapped | ❌ | ✅ | ✅ |
algorithmFamily key mismatches (7) |
❌ | ✅ | ✅ |
RSA-PSS → RSASSA-PSS |
❌ | ✅ | ✅ |
RSA-KEM invalid family value |
— | ❌ | ✅ (acknowledged as null) |
| 27 dead curve keys in family map | — | ✅ | |
| SHA-224 / SHA-512/224 / SHA-512/256 gaps | — | ✅ | |
EC- fallback wrong for Curve25519/448, Edwards25519/448 |
— | — | ❌ New |
First EC- block is dead code |
— | — | ❌ New |
Bare SHA3 has no family |
|||
Test coverage of algorithmFamily |
❌ thin | ✅ comprehensive (bar the above) | |
specVersion "1.7" emitted |
✅ | ✅ | ✅ |
| Deprecated fields suppressed | ✅ | ✅ | ✅ |
relatedCryptographicAssets correct |
✅ | ✅ | ✅ |
6c6f0bc to
31297da
Compare
|
@san-zrl Both bugs have been fixed. Full verification passed (mvn clean package green, 21/21 AlgorithmTest tests pass, checkstyle/spotless clean, working tree clean).
Hoping for green but any other suggestions and recommendations will be highly appreciated... :)) |
san-zrl
left a comment
There was a problem hiding this comment.
Hi @sachin9058 - We're getting there :-) Please remove the duplicate keys and correct the PR description on certificateFileExtension, serialNumber, and fingerprint. There's nothing you can do (see below)
PR-543 Final Evaluation — v4
✅ All green
specVersion: "1.7"— emitted correctly viaVERSION_17andCycloneDx17JsonGenerator.- Deprecated fields suppressed —
curve,cryptoRefArray,certificateExtension,signatureAlgorithmRef,subjectPublicKeyRefall hidden by MixIns. - All 23 curves mapped to correct CycloneDX 1.7 registry IDs (
nist/,secg/,brainpool/,other/). - All algorithm families resolve correctly, including the
EC-prefix fallback forEllipticCurveAlgorithm, and previously broken entries (EdDSA, ML-KEM, ML-DSA, SHA-1/2/3, scrypt, RSA-PSS). relatedCryptographicAssetsreplacescryptoRefArraywithtype = "algorithm".- Test coverage is comprehensive for all of the above.
⚠️ Duplicate keys in the static initialiser
The static {} block in AlgorithmComponentBuilder.java is split into two sequential sections. The first section (lines 124–194) contains the original algorithm entries plus a first set of curve-name keys. The second section (lines 195–233) was appended in a later commit and re-declares 36 of those same keys with the same values: Salsa20, SHA-1, SHA-256/384/512, SHA3 and all its digest-size variants, and all 27 curve-name keys.
HashMap.put() on a duplicate key silently overwrites, so since both occurrences map to the same value the runtime result is correct today. The risk is forward-looking: if a future change updates the family for, say, "Edwards25519" in the first occurrence but misses the second, the second put wins and silently reverts the change. There is no compile error, no test failure, and no warning. The fix is to delete lines 195–233 and add only the three genuinely new entries (SHA-224, SHA-512/224, SHA-512/256) in the first block.
❌ One false claim
certificateFileExtension, serialNumber, and fingerprint are stated as added to certificate properties. They do not exist in cyclonedx-core-java 13.2.0's CertificateProperties and no code sets them. This is a library limitation, not a code bug.
31297da to
dc48e24
Compare
|
@san-zrl Done. and I think i have taken too many iterations on this PR. Please take a look and tell me if it looks good. :-)) |
san-zrl
left a comment
There was a problem hiding this comment.
Hi @sachin9058 - You made it! Thank you very much for your contribution. This is appreciated since the upgrade to Cyclone DX 1.7 has been on our todo list for quite a while. Thanks also for the quick responses to my comments.
PR-543 Final Evaluation — v5
✅ All green
specVersion: "1.7"— emitted correctly viaVERSION_17andCycloneDx17JsonGenerator. Redundant reflection insaveTo()is harmless.- Deprecated fields suppressed —
curve,cryptoRefArray,certificateExtension,signatureAlgorithmRef,subjectPublicKeyRefall hidden by MixIns. - All 23 curves mapped to correct CycloneDX 1.7 registry IDs (
nist/,secg/,brainpool/,other/). - All algorithm families resolve correctly. The
EC-prefix fallback strips"EC-"and looks up the curve name, giving the right family for all curve types (ECDSA for secp/sect/brainpool, ECDH for Curve25519/448, EdDSA for Edwards25519/448). relatedCryptographicAssetsreplacescryptoRefArraywithtype = "algorithm".- Duplicate keys fixed — the static initialiser is now a single clean block (lines 99–191), no key appears twice.
- Test coverage is comprehensive for all of the above.
❌ One false claim (library limitation)
certificateFileExtension, serialNumber, and fingerprint are stated as added to certificate properties. They do not exist in cyclonedx-core-java 13.2.0's CertificateProperties. No code sets them. Cannot be fulfilled at this library version.
|
@san-zrl Hi andreas if can u just merge PR then i can start working on preparing next pr. Like if any changes are required just let me know so that i can fix them ASAP... |
There was a problem hiding this comment.
@sachin9058 thanks you! I think we should revise the implementation approach first: high-level, I think, the mapper model should own each algorithm's canonical family, using its kind and parameters where needed. The output module should translate that model value to the CycloneDX 1.7 registry identifier. The current name-based table duplicates model knowledge and produces incorrect RSA/EC families while missing X25519/X448.
dc48e24 to
de226de
Compare
Bump cyclonedx-core-java to 13.2.0 and emit specVersion 1.7, validated against bom-1.7.schema.json in the output tests. - Algorithms report their canonical family through IAlgorithm#getFamily, derived from kind and parameters (e.g. RSA encryption, OAEP and signature, EC curves, x25519/x448). The output module only translates the model value to the CycloneDX registry identifier. - ellipticCurve is emitted only for registry identifiers, so no invalid enum value can be written. The deprecated curve and cryptoRefArray are still populated so 1.6 consumers keep working. - Protocols list their assets in relatedCryptographicAssets, resolved from the merged dependency graph so repeated detections are included. - Key wrapping is serialized with the 1.7 key-wrap primitive. Signed-off-by: Sachin Kumar <sachinkumar905846@gmail.com>
de226de to
8fce3df
Compare
|
Thanks @n1ckl0sk0rtge, I reworked the PR from scratch on top of current main. Point by point:
One known gap: only about 70 algorithm classes declare a family so far. The others emit no |
Summary
CBOM output is now CycloneDX 1.7 (
specVersion: "1.7"), built on cyclonedx-core-java 13.2.0.Following the discussion in #505, 1.7 output still populates the deprecated 1.6 fields, so
existing 1.6 consumers keep working. There is no version switch.
Fixes #505 (together with the open points below)
What changed
algorithmFamily: the mapper model now owns the canonical family.AlgorithmFamilyandIAlgorithm#getFamily()are new, and about 70 algorithm classes declare their family.RSAES-OAEP or RSASSA-PKCS1. RSA-PSS gives RSASSA-PSS.
EdDSA, and other curves give ECDSA or ECDH by kind.
x25519andx448are covered. Families survive recasts and copies.CycloneDxRegistrymaps model values to registry identifiers. Onlyidentifiers from
cryptography-defs.schema.jsoncan be emitted.ellipticCurve: the 23 model curves map to registry identifiers (nist/P-256,secg/sect163k1,brainpool/brainpoolP256r1,other/Ed25519, ...). An unknown curve emitsno
ellipticCurve. The deprecated free-textcurveis still populated.relatedCryptographicAssets(typed, e.g.algorithm) is added alongside thedeprecated
cryptoRefArray. Both are resolved from the merged dependency graph, so a protocoldetected several times lists the assets of all its occurrences. Previously later detections
were lost.
KeyWrapalgorithms serialize"primitive": "key-wrap". The library enum has nosuch constant yet, so
AlgorithmProperties17provides it. It can be removed once the libraryadds
KEY_WRAP.Not included
certificateFileExtension,serialNumberandfingerprintdo not exist in cyclonedx-core-java13.2.0, and the plugin emits no certificates.
algorithmFamily. More can be added incrementally.Tests
bom-1.7.schema.json.CycloneDx17Testcovers:curvesaveTo()AlgorithmFamilyand every model curve resolves to a value in the schema enumsmvn clean packagepasses for all modules.