Skip to content

Removal of Enable Dilithium Flag#2096

Closed
jakemas wants to merge 12 commits intoaws:mainfrom
jakemas:mldsa-flag-2
Closed

Removal of Enable Dilithium Flag#2096
jakemas wants to merge 12 commits intoaws:mainfrom
jakemas:mldsa-flag-2

Conversation

@jakemas
Copy link
Copy Markdown
Contributor

@jakemas jakemas commented Jan 6, 2025

Issues:

It's time. (also we need to do this to add ML-DSA to the FIPS module)

Description of changes:

  • Removed the enable_dilithium flag
  • Added a description to APIs to indicate the external APIs may still be subject to change as the standards space develops. (We don't envision them changing, but they are extremely new).

Call-outs:

Removing the flag has little consequence -- other than it makes the APIs we expose in include/openssl/evp.h that much more "final". We should consider how much we like them before we commit to them. We made a point to refer to asymmetric keypairs as public and private keys, rather than public and secret keys -- but there is inconsistency with EVP_PKEY_kem_new_raw_secret_key. After internal discussions, we'd also like to change the name of EVP_PKEY_kem_new_raw_public_key and EVP_PKEY_kem_new_raw_secret_key to EVP_PKEY_kem_new_raw_encapsulation_key and EVP_PKEY_kem_new_raw_decapsulation_key to match the names within FIPS 203. Thus, we should stick with private.

We did consider the use of an experimental flag (by using the OPENSSL_DEPRECATED alias), but, this seems a little over the top. Alternatively, we could place the EVP APIs in include/openssl/experimental/. The arguments of EVP_PKEY_pqdsa_new_raw_secret_key, EVP_PKEY_pqdsa_new_raw_public_key, and EVP_PKEY_CTX_pqdsa_set_params are stable and consistant with KEM and EC variants (see EVP_PKEY_CTX_kem_set_params, EVP_PKEY_CTX_set_dh_paramgen_prime_len, and EVP_PKEY_CTX_set_ec_param_enc, EVP_PKEY_new_raw_private_key, EVP_PKEY_new_raw_public_key, EVP_PKEY_kem_new_raw_public_key).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@jakemas jakemas requested a review from a team as a code owner January 6, 2025 22:15
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.93%. Comparing base (3cea179) to head (7a9cb06).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2096      +/-   ##
==========================================
+ Coverage   78.75%   78.93%   +0.18%     
==========================================
  Files         598      610      +12     
  Lines      103657   105195    +1538     
  Branches    14720    14908     +188     
==========================================
+ Hits        81637    83039    +1402     
- Misses      21368    21503     +135     
- Partials      652      653       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jakemas jakemas changed the title [TEST] Removal of Enable Dilithium Flag Removal of Enable Dilithium Flag Jan 7, 2025
@jakemas
Copy link
Copy Markdown
Contributor Author

jakemas commented Jan 10, 2025

PAUSE on this PR. Removing flag in #2105

@jakemas jakemas closed this Jan 14, 2025
andrewhop pushed a commit that referenced this pull request Feb 11, 2025
…errors (#2177)

### Issues:
Resolves CI failures around delocator in
#2175


### Description of changes: 
The FIPS static builds for ARM are failing CI:

```
[57%] Building ASM object crypto/fipsmodule/CMakeFiles/bcm_hashunset.dir/bcm-delocated.S.o
bcm.c: Assembler messages:
bcm.c:125130: Error: shift expression expected at operand 2 -- movi v4.4s,0xf,.Lbcm_redirector_msl,8' bcm.c:125133: Error: shift expression expected at operand 2 -- mvni v3.4s,0x1f,.Lbcm_redirector_msl,8'
bcm.c:130233: Error: shift expression expected at operand 2 -- movi v4.4s,0xf,.Lbcm_redirector_msl,8' bcm.c:130236: Error: shift expression expected at operand 2 -- mvni v3.4s,0x1f,.Lbcm_redirector_msl,8'
bcm.c:190061: Error: shift expression expected at operand 2 -- movi v4.4s,0xf,.Lbcm_redirector_msl,8' bcm.c:190064: Error: shift expression expected at operand 2 -- mvni v3.4s,0x1f,.Lbcm_redirector_msl,8'
bcm.c:206492: Error: shift expression expected at operand 2 -- movi v4.4s,0xf,.Lbcm_redirector_msl,8' bcm.c:206495: Error: shift expression expected at operand 2 -- mvni v3.4s,0x1f,.Lbcm_redirector_msl,8'
```


This was discussed back in #2096 but
not merged due to the PR being closed.


The Arm instruction has a special argument `msl`
(https://developer.arm.com/documentation/100069/0606/SIMD-Vector-Instructions/MOVI--vector-).
The delocator is intepreting `msl` as a function (or global symbol).
Since it’s not defined in the fipsmodule, it creates a jump function
`bcm_redirector_msl` outside the fipsmodule that removes a potential
relocation. The suffix of `bcm_redirector_msl` is the original token,
which matches `msl`.

We fix by adding `msl` to the list of `ARMConstantTweak` and regenerate
the `delocate.peg.go` file.

After testing this fix, I begin to see a second error in
`amazonlinux2023_clang15x_aarch_fips`:

```
[339/575] Generating bcm-delocated.S
FAILED: crypto/fipsmodule/bcm-delocated.S /codebuild/output/src1965601223/src/github.com/aws/aws-lc/test_build_dir/crypto/fipsmodule/bcm-delocated.S 
panic: Symbol reference outside of ldr instruction [recovered]
    panic: error while processing "\tldrsw\tx9, [x9, :lo12:ml_dsa_zetas+4]\n" on line 120399: "Symbol reference outside of ldr instruction"
```

It seems that `ldrsw` is not recognised as `ldr`. We already have
`ldrsw` in our code, so we extend the delocate.go to accept `ldrsw` in
addition to `ldr`.


### Call-outs:
Unblocks progress on work in the FIPS module, such as ED25519 and
ML-DSA.


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants