feat: verbose output utility for policy builder #5502

jouho · 2025-09-10T23:55:40Z

Release Summary:

Resolved issues:

Description of changes:

Adds a utility function to the policy builder for outputting a breakdown of a security policy. We define a V1 format that we currently use for the policy snapshot utility, and replace it to use this policy builder utility instead. I essentially moved the bin/policy.c utility into the policy builder. In the long term, we plan to add more formalized formats which we can add as V2, V3, etc .

Call-outs:

I ran the following command to regenerate all the snapshots and verify that the new policy output utility is working:

./tests/policy_snapshot/generate.sh build/bin/policy ./tests/policy_snapshot/snapshots

As a result, some snapshots were updated.

Testing:

Added unit tests for happy paths as well as negative paths. We have snapshot test that checks for exact content match.

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

jouho · 2025-09-11T22:39:53Z

tests/policy_snapshot/snapshots/ELBSecurityPolicy-2016-08

@@ -1,4 +1,4 @@
-name: ELBSecurityPolicy-2016-08
+name: ELBSecurityPolicy-TLS-1-0-2015-05


While this seems confusing, it's expected behavior because the tool searches the security_policy_selection array to find the first matching policy object. Although the names are different, they both point to the same exact policy configuration.

This might be confusing, but I don't think it will be an issue for someone trying to understand the components of the security policy. I considered two alternatives:

List all aliases: Show all policy names that map to the same policy object. This would be useful for identifying equivalent policies but comes with a performance cost.

Retain the user's input: Store whatever the user requested initially when building the policy builder and use that value for the name. This would require adding a new field to the s2n_security_policy_builder struct, but since we don't have other use cases for that field, I'm not sure if we want to commit to it yet.

I'd appreciate any opinions on this.

lrstewart · 2025-09-12T03:52:26Z

tls/policy/s2n_policy_feature.h

+     * FORMAT_V1: Human-readable YAML-style format
+     * 


Is this really YAML? If it is, it's a coincidence 😅

If we're claiming this is YAML should we try to parse the results with a YAML parser maybe?

lrstewart · 2025-09-12T03:55:01Z

tls/policy/s2n_policy_feature.h

+ * @param fd The file descriptor to write to (e.g., STDOUT_FILENO or an open file)
+ * @returns S2N_SUCCESS on success, S2N_FAILURE on failure
+ */


Do you think writing to a file descriptor is sufficient? Would customers want to write to memory instead? This might be particularly important bc I don't believe any of our current APIs write to a file descriptor yet.

lrstewart · 2025-09-12T03:55:35Z

tls/policy/s2n_policy_feature.h

+    /* Future formats can be added here */
+} s2n_policy_format;


Nit: Idk about this placeholder comment in what will eventually be a public enum

lrstewart · 2025-09-12T03:56:48Z

tls/policy/s2n_policy_feature.h

+     * - rules:
+     *   - <rule_name>: <yes|no>


Would true/false be more normal, if we're calling this YAML?

lrstewart · 2025-09-12T03:57:47Z

tls/policy/s2n_policy_feature.h

+int s2n_policy_builder_write_verbose(struct s2n_security_policy_builder *builder,
+        s2n_policy_format format, int fd);


You should be operating on the s2n_security_policy itself, not the builder. We want to print the actual policies, and we want it to be applicable to policies regardless of origin.

lrstewart · 2025-09-12T03:59:36Z

tls/policy/s2n_policy_builder.c

+    /* TODO: Currently outputs the base_policy. This needs to be updated once builder implements a "finalized" policy field */
+    const char *policy_name = "unknown";
+    for (size_t i = 0; security_policy_selection[i].version != NULL; i++) {
+        if (security_policy_selection[i].security_policy == builder->base_policy) {
+            policy_name = security_policy_selection[i].version;
+            break;
+        }
+    }


The goal here isn't to print existing, hard-coded policies (except to share code with bin/policy.c). The goal is to print the newly constructed policies, to see the results of the builder. That means you cannot rely on security_policy_selection. The new policy won't be in security_policy_selection.

goatgoose · 2025-09-12T19:08:40Z

tls/policy/s2n_policy_feature.h

+     *   - kem groups:
+     *     -- <kem_group_name>
+     */
+    S2N_POLICY_FORMAT_V1 = 1,


What is the purpose of version numbers for the policy formats? Is the thought that we can't update our format at all because we might break users? Does that mean we'll potentially have two very similar YAML outputs if we want to make a minor change to V1?

In this case I wonder if it'd be worth giving the formats a name in addition to the version? Like

S2N_POLICY_FORMAT_YAML_V1 = 1, S2N_POLICY_FORMAT_YAML_V2 = 2, S2N_POLICY_FORMAT_XML_V1 = 3,

Otherwise it might be strange to have V1 be YAML, V2 be some completely other thing, and V3 be a slight modification to the V1 YAML?

goatgoose · 2025-09-12T19:13:34Z

tls/policy/s2n_policy_builder.c

I'm wondering if policy_builder is the right place for this stuff. It seems like it might pollute the actual security policy builder logic if we add a bunch of new output formats? Maybe s2n_policy_writer.c or something? Not sure.

goatgoose · 2025-09-12T21:27:02Z

tls/policy/s2n_policy_builder.c

+
+    POSIX_GUARD(s2n_write_fd_formatted(fd, "name: %s\n", policy_name));
+
+    const char *version_str = version_strs[policy->minimum_protocol_version];


Might be good to check that policy->minimum_protocol_version isn't too large for version_strs. Though unlikely, I could see special values being used to mean "any version" or something, and we wouldn't want to over-read version_strs.

goatgoose · 2025-09-12T21:29:45Z

tls/policy/s2n_policy_builder.c

+}
+
+/* Write verbose policy output in FORMAT_V1 style */
+static int s2n_policy_write_format_v1(const struct s2n_security_policy *policy, const char *policy_name, int fd)


Ideally internal functions would return S2N_RESULT. See

s2n-tls/utils/s2n_result.h

Line 19 in 792d366

* The goal of s2n_result is to provide a strongly-typed error

goatgoose · 2025-09-12T21:36:44Z

tls/policy/s2n_policy_feature.h

+     * FORMAT_V1: Human-readable YAML-style format
+     * 


If we're claiming this is YAML should we try to parse the results with a YAML parser maybe?

goatgoose · 2025-09-12T21:50:26Z

tests/unit/s2n_policy_builder_test.c

+    RESULT_ENSURE(buffer_size > 0, S2N_ERR_INVALID_ARGUMENT);
+
+    /* Use a pipe for capturing output */
+    int pipe_fds[2];


The compiler might not initialize your variables for you, so it's best to do it manually. Having your variables initialized with garbage can lead to undefined behavior.

Suggested change

int pipe_fds[2];

int pipe_fds[2] = { 0 };

feat: verbose output utility for policy builder

2dd4349

github-actions bot added the s2n-core team label Sep 10, 2025

jouho and others added 5 commits September 11, 2025 18:06

feat: verbose output utility for policy builder

f7cc36b

Merge branch 'main' into verbose-output

3cab974

address cpp check

2db93cd

address cpp check

6b9c51f

Merge branch 'main' into verbose-output

ad6df26

jouho commented Sep 11, 2025

View reviewed changes

jouho marked this pull request as ready for review September 11, 2025 22:55

jouho requested review from goatgoose and lrstewart September 11, 2025 22:55

lrstewart reviewed Sep 12, 2025

View reviewed changes

goatgoose reviewed Sep 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: verbose output utility for policy builder #5502

feat: verbose output utility for policy builder #5502

Uh oh!

jouho commented Sep 10, 2025 •

edited

Loading

Uh oh!

jouho Sep 11, 2025 •

edited

Loading

Uh oh!

lrstewart Sep 12, 2025

Uh oh!

goatgoose Sep 12, 2025

Uh oh!

lrstewart Sep 12, 2025

Uh oh!

lrstewart Sep 12, 2025

Uh oh!

lrstewart Sep 12, 2025

Uh oh!

lrstewart Sep 12, 2025

Uh oh!

lrstewart Sep 12, 2025

Uh oh!

goatgoose Sep 12, 2025

Uh oh!

goatgoose Sep 12, 2025

Uh oh!

goatgoose Sep 12, 2025

Uh oh!

goatgoose Sep 12, 2025

Uh oh!

goatgoose Sep 12, 2025

Uh oh!

goatgoose Sep 12, 2025

Uh oh!

Uh oh!

		@@ -1,4 +1,4 @@
		name: ELBSecurityPolicy-2016-08
		name: ELBSecurityPolicy-TLS-1-0-2015-05

		int s2n_policy_builder_write_verbose(struct s2n_security_policy_builder *builder,
		s2n_policy_format format, int fd);


		POSIX_GUARD(s2n_write_fd_formatted(fd, "name: %s\n", policy_name));

		const char *version_str = version_strs[policy->minimum_protocol_version];

feat: verbose output utility for policy builder #5502

Are you sure you want to change the base?

feat: verbose output utility for policy builder #5502

Uh oh!

Conversation

jouho commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Summary:

Resolved issues:

Description of changes:

Call-outs:

Testing:

Uh oh!

jouho Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jouho commented Sep 10, 2025 •

edited

Loading

jouho Sep 11, 2025 •

edited

Loading