Skip to content

Add a reader for the system crypto-policies OpenSSL back-end - #3502

Merged
WillChilds-Klein merged 7 commits into
mainfrom
feat/crypto-policies-2-parser
Sep 16, 2026
Merged

WillChilds-Klein merged 7 commits into
mainfrom
feat/crypto-policies-2-parser

Conversation

@WillChilds-Klein

@WillChilds-Klein WillChilds-Klein commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Stack, split out of #3442, to merge bottom-up (#3501, the error-queue primitives, has landed):

  1. Add a reader for the system crypto-policies OpenSSL back-end #3502 -- policy file reader (this PR)
  2. Export the crypto-policies reader and add its CI job #3527 -- shared-build symbol export and CI
  3. Seed cipher lists and version bounds from the system crypto policy #3503 -- cipher lists and version bounds
  4. Seed groups and signature algorithms from the system crypto policy #3504 -- groups and signature algorithms
  5. Keep AWS-LC's post-quantum defaults unless the policy opts out #3505 -- post-quantum defaults

Description

  • Adds an off-by-default build flag, -DENABLE_CRYPTO_POLICIES, and a reader for the OpenSSL back-end file that Amazon Linux 2023 and Fedora render from the operator's chosen system policy.
  • AWSLC_CRYPTO_POLICY_FILE relocates that file at build time and is declared in the CMake cache, so cmake -L and cmake-gui list it for a packager who is not reading the CMakeLists.
  • Parses the directives AWS-LC could act on into a fixed-size config struct. Nothing consumes the result yet.
  • Comments, section headers, and unknown keys are ignored, so the reader tolerates the rest of what the framework writes today and whatever it adds later. A value too long to represent is ignored the same way, even where an earlier line set the same key.
  • The reader succeeds only if it read the whole file, so half a policy cannot pass for a shorter one.
  • The policy path is fixed at build time and overridable at runtime, which is how the tests here and in the rest of the stack drive it. The override is dropped in a secure execution, where the environment sits on the far side of a privilege boundary from the root-owned default path.

Testing / verification

  • Parse tests cover a full stock policy, quoting, surrounding whitespace, a repeated key, and a final line with no trailing newline.
  • An overlong value or line is dropped whole rather than truncated, and drops the value an earlier line gave the same key, so a policy larger than the reader's buffers cannot quietly become a different policy.
  • A missing file, a read error, and null arguments all fail rather than yielding a half-filled config. The read-error case opens a directory, which fails on the first read rather than on the open.
  • The runtime path override is exercised directly, since every later test in the stack rests on it.
  • Checked the override drop against a real binary: honored as an ordinary process, ignored once that binary carries file capabilities, which leave the real and effective ids equal.

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.

skmcgrail
skmcgrail previously approved these changes Sep 9, 2026
@WillChilds-Klein
WillChilds-Klein added this pull request to stack #3509 September 9, 2026 22:50
justsmth
justsmth previously approved these changes Sep 11, 2026
Comment thread ssl/crypto_policy.cc
Comment thread ssl/crypto_policy.cc Outdated
Base automatically changed from feat/crypto-policies-1-err-primitives to main September 11, 2026 15:03
WillChilds-Klein added a commit that referenced this pull request Sep 11, 2026
Stack, split out of #3442, to merge bottom-up:

1. **#3501 -- error-queue primitives (this PR)**
2. #3502 -- policy file reader
3. #3503 -- cipher lists and version bounds
4. #3504 -- groups and signature algorithms
5. #3505 -- post-quantum defaults

## Description

- Adds `ERR_num_errors` and `ERR_pop_to_count`, so code that calls into
libcrypto on a caller's behalf can drop the errors it raised and leave
the queue it was handed untouched.
- The existing mark APIs cannot do this. `ERR_set_mark` needs an entry
to mark, so it is a no-op on an empty queue, and popping to a mark
consumes one the caller had already set.
- `ERR_clear_error` and `ERR_restore_state` rebuild the queue, which
dangles the data pointer the caller got from its last
`ERR_get_error_line_data`.
- A count is a position rather than a mark, so it nests inside a
caller's mark without disturbing it.

## Testing / verification

- New error-queue tests cover popping back to a recorded count, a count
taken from an empty queue, and a count at or above the queue's length.
- One case fills the ring past its capacity to confirm a stale count
leaves the caller's errors alone.
- One case wraps a nested call in the caller's own mark and an error
carrying a data string, then checks the mark still pops and the string
is intact.
- One case pins that a mark does not survive
`ERR_save_state`/`ERR_restore_state`, since a snapshot can be restored
many times and would re-arm a mark nobody set.

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.
Amazon Linux 2023 and Fedora render the operator-selected system TLS posture to
an OpenSSL back-end config file, which AWS-LC ignores. This reads that file
behind an off-by-default build flag; nothing consumes the result yet.
Both are now AWSLC_CRYPTO_POLICY_FILE; the macro holding the value they fall
back to is AWSLC_CRYPTO_POLICY_DEFAULT_FILE, so no identifier means two things.
@WillChilds-Klein
WillChilds-Klein dismissed stale reviews from justsmth and skmcgrail via c1639d5 September 11, 2026 15:03
@WillChilds-Klein
WillChilds-Klein force-pushed the feat/crypto-policies-2-parser branch from 46b9dc9 to c1639d5 Compare September 11, 2026 15:03
@codecov-commenter

codecov-commenter commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.41%. Comparing base (973b592) to head (c8c14aa).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3502      +/-   ##
==========================================
+ Coverage   78.37%   78.41%   +0.03%     
==========================================
  Files         700      700              
  Lines      125744   125758      +14     
  Branches    17388    17384       -4     
==========================================
+ Hits        98553    98612      +59     
+ Misses      26318    26275      -43     
+ Partials      873      871       -2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown
Contributor

🔒 Security ReviewView Report

Please review before merging.

fgets stops on error the same way it stops at EOF, so a file that broke part
way through was reported as a policy that simply ended.
Comparing real and effective ids misses AT_SECURE cases that leave them equal:
a binary with file capabilities, or a set-uid program that already dropped
privileges.
justsmth
justsmth previously approved these changes Sep 11, 2026
@WillChilds-Klein
WillChilds-Klein removed this pull request from stack #3509 September 14, 2026 15:45
@WillChilds-Klein
WillChilds-Klein changed the base branch from main to feat/crypto-policies-1-err-primitives September 14, 2026 16:10
@WillChilds-Klein
WillChilds-Klein added this pull request to stack #3509 September 14, 2026 16:11
@WillChilds-Klein
WillChilds-Klein dismissed justsmth’s stale review September 14, 2026 16:11

The merge-base changed after approval.

@WillChilds-Klein
WillChilds-Klein removed this pull request from stack #3509 September 14, 2026 16:16
@WillChilds-Klein
WillChilds-Klein changed the base branch from feat/crypto-policies-1-err-primitives to main September 14, 2026 16:16
@WillChilds-Klein
WillChilds-Klein added this pull request to stack #3528 September 14, 2026 16:18
justsmth
justsmth previously approved these changes Sep 14, 2026
// Half a group list is not a weaker version of the operator's policy, it is a
// different policy nobody chose, so an over-long value is dropped whole and the
// field left empty.
TEST(CryptoPolicyParseTest, OverlongValueIsDroppedNotTruncated) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a gap in behavior for the scenario where there is a valid value, and then an over-long value following it. The old valid value is retained and does not cause it to be treated as "not-present" based on the last key winning logic.

example: "Groups = X25519\nGroups = <1025 chars>"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so in that case, we'd "fail open" and fall back to AWS-LC's defualts. over-large value is an unlikely edge case; that behavior seems appropriate to me.

The last occurrence of a key is the operator's choice. Leaving the
previous value in place when that one will not fit applies a policy they
replaced.
@WillChilds-Klein
WillChilds-Klein merged commit 6d8a1d6 into main Sep 16, 2026
622 of 631 checks passed
@WillChilds-Klein
WillChilds-Klein deleted the feat/crypto-policies-2-parser branch September 16, 2026 04:11
WillChilds-Klein added a commit that referenced this pull request Sep 16, 2026
Stack, split out of #3442, to merge bottom-up (#3501, the error-queue
primitives, has landed):

1. #3502 -- policy file reader
2. **#3527 -- shared-build symbol export and CI (this PR)**
3. #3503 -- cipher lists and version bounds
4. #3504 -- groups and signature algorithms
5. #3505 -- post-quantum defaults

## Description

- Exports the two policy-reader internals the tests call and registers
them in the libssl symbol registry. A shared build needs both: hidden
visibility keeps them out of the library, and the version script an
`ENABLE_DIST_PKG` build applies keeps them out again.
- Runs the symbol extractor once more with the crypto-policies build
flag defined, so declarations sitting behind that guard reach the
registry at all.
- Adds the Amazon Linux 2023 CI job for the feature: the suite with the
flag on in stock CMake and in the shared, symbol-versioned
`ENABLE_DIST_PKG` build a distribution packages, a run against the
policy file the system renders, and a build with libssl off.
- The job's seeding-specific parts, the neutralizing path override and
the require-system flag, are inert until #3503 adds seeding and the test
hook that reads them.
- The build flag stays off by default, so no shipped configuration
changes.

## Testing / verification

- Ran both build configurations the job runs and the policy tests in
each. Only the distribution one fails to link without this change, which
is why a stock build alone let the gap through.
- Reverting only the generated version script brings the undefined
references back, so the registry entries earn their place alongside the
export attribute.
- Regenerating the version script from the registry is byte-identical,
the invariant the symbol-check job's `mapcheck` mode enforces.

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