Add constexpr security descriptor building by jonwis · Pull Request #628 · microsoft/wil

jonwis · 2026-04-01T23:14:16Z

We have a lot of code in various startup paths that uses ConvertStringSecurityDescriptorToSecurityDescriptor, or worse, does hand-building of SECURITY_DESCRIPTOR with building all the ACEs, parsing SID strings, etc.

This introduces a constexpr SD builder, skipping all that and building a self-relative SD at compile time. So now you can say things like:

constexpr auto sd = wil::make_self_relative_sd(
        wil::sd_owner<"S-1-5-32-544">(),                     // BUILTIN\Administrators
        wil::sd_group<"S-1-5-32-545">(),                     // BUILTIN\Users
        wil::make_deny_ace<"S-1-5-7">(GENERIC_WRITE),        // deny ANONYMOUS LOGON
        wil::make_allow_ace<"S-1-5-11">(GENERIC_READ));       // allow Authenticated Users

…elf_relative_sd(), make_allow_ace(), make_deny_ace(),\nsd_owner(), sd_group(), and no_sid for composing self-relative\nsecurity descriptors at compile time from static SIDs and typed\nACEs. C++20 only, gated behind _HAS_CXX20.\n\nSupports allow/deny ACE types, per-ACE inheritance flags, and\noptional owner/group SIDs. Deny-before-allow ordering enforced\nvia WI_ASSERT."

…n\nParse SID string literals (e.g. \"S-1-5-32-544\") into static_sid_t\nat compile time via C++20 class-type NTTPs. The resulting SID works\nwith all existing helpers: sd_owner, sd_group, make_allow_ace, etc.\n\nGuarded behind __WIL_HAS_CLASS_NTTP which checks for class-type\nNTTP support on both MSVC and Clang."

dmachaj · 2026-04-02T00:32:04Z

+        size_t count = 0;
+        for (size_t i = 0; i < S.length; ++i)
+        {
+            if (S.data[i] == '-')


[nit] Style standard is usually to always have { } even for single line if blocks.

Fair - i'll ask GHCP why it didn't follow the clang-format conventions.

Looks like InsertBraces: true was added in Clang 15, but our .clang-format doesn't include it.

dmachaj · 2026-04-02T00:34:10Z

+        uint64_t value = 0;
+        while (pos < len && str[pos] >= '0' && str[pos] <= '9')
+        {
+            value = value * 10 + static_cast<uint64_t>(str[pos] - '0');


I had to glare at this for a minute to be sure order of operations was intended. Consider adding ( ) around the multiply to make it obviously-intentional.

dunhor · 2026-04-02T17:25:28Z

+        static constexpr size_t byte_size()
+        {
+            return 8 + 4 * AuthorityCount;
+        }


This should be a constexpr variable... no reason for a function here.

e.g. similar to how npos works

dunhor · 2026-04-02T17:29:23Z

    return make_static_sid(SECURITY_NT_AUTHORITY, wistd::forward<Ts>(subAuthorities)...);
 }

+#ifdef _HAS_CXX20


Don't rely on this macro. Use __WI_LIBCPP_STD_VER

dunhor · 2026-04-02T17:35:55Z

+#ifdef _HAS_CXX20
+
+// Class-type NTTPs: MSVC defines __cpp_nontype_template_args; Clang supports it in C++20+ mode without the macro.
+#if __cpp_nontype_template_args >= 201911L || (defined(__clang__) && __clang_major__ >= 12 && _HAS_CXX20)


From https://releases.llvm.org/18.1.6/tools/clang/docs/ReleaseNotes.html#c-20-feature-support:

Implemented P1907R1 which extends allowed non-type template argument kinds with e.g. floating point values and pointers and references to subobjects. This feature is still experimental. Accordingly, __cpp_nontype_template_args was not updated. However, its support can be tested with __has_extension(cxx_generalized_nttp).

Sounds like it's support is experimental. That's probably okay, though it does give me pause. In any case, we should possibly do the check it suggests instead of querying for the version

Yeah, I dunno - I thought it could do something similar to how format works. I need to goad it harder probably.

dunhor · 2026-04-02T17:43:17Z

+#ifdef _HAS_CXX20
+
+// Class-type NTTPs: MSVC defines __cpp_nontype_template_args; Clang supports it in C++20+ mode without the macro.
+#if __cpp_nontype_template_args >= 201911L || (defined(__clang__) && __clang_major__ >= 12 && _HAS_CXX20)


You also make use of consteval and should probably check __cpp_consteval as well

dunhor · 2026-04-02T17:47:10Z

+
+    // Count '-' characters in a fixed_string.
+    template <fixed_string S>
+    consteval size_t count_dashes()


Does this need to be consteval? Also any reason not to make this a member function?

And if you change them to be constexpr should probably make them all noexcept for good hygiene

dunhor · 2026-04-02T17:53:02Z

+    }
+
+    // Parse an unsigned decimal integer from a string starting at pos, advancing pos past the digits.
+    consteval uint64_t parse_uint(const char* str, size_t len, size_t& pos)


Probably another function that doesn't need to be consteval.

Interesting side note: looks like std::from_chars became constexpr in C++23. Obviously can't rely on that here, just interesting.

dunhor · 2026-04-02T18:05:34Z

+{
+    static_assert(sizeof...(aces) > 0, "at least one ACE is required");
+
+    constexpr size_t sdHeaderSize = 20; // SECURITY_DESCRIPTOR_RELATIVE header


Is there an issue with using sizeof(SECURITY_DESCRIPTOR_RELATIVE) that's not immediately obvious?

I guess it's because of the manual math when writing down below. Probably still good for a static_assert though

dunhor · 2026-04-14T17:57:45Z

+
+    // Parse a SID string "S-1-{authority}-{sub1}-{sub2}-..." into a static_sid_t.
+    template <fixed_string S>
+    consteval auto parse_sid_string()


Seems more ergonomic to make this a static from_string or something member of static_sid_t. You'd probably need to move some definitions around a bit though

dunhor · 2026-04-14T18:00:37Z

+    {
+        // "S-R-IA" has 2 dashes; each sub-authority adds one more.
+        constexpr size_t subAuthCount = count_dashes<S>() - 2;
+        static_assert(subAuthCount <= SID_MAX_SUB_AUTHORITIES, "too many sub authorities in SID string");


A potentially confusing error message if you get integer underflow (i.e. fewer than 2 dashes)

dunhor · 2026-04-14T18:39:27Z

+        // Expect 'S' or 's'
+        ++pos; // skip 'S'
+        ++pos; // skip '-'


Would be nice to check things like this, and the dashes below (throw exceptions to fail at compile time). Probably unlikely, but we all make silly mistakes from time to time

dunhor · 2026-04-14T18:42:13Z

+        ++pos; // skip '-'
+
+        // Revision
+        result.Revision = static_cast<uint8_t>(parse_uint(S.data, S.length, pos));


Maybe templatize the integer type for parse_uint.

Would also be ideal if things like integer overflow could fail the build

dunhor · 2026-04-14T18:48:33Z

+    constexpr void write_byte(uint8_t* dest, size_t& offset, uint8_t value)
+    {
+        dest[offset++] = value;
+    }
+
+    constexpr void write_word(uint8_t* dest, size_t& offset, uint16_t value)
+    {
+        dest[offset++] = static_cast<uint8_t>(value & 0xFF);
+        dest[offset++] = static_cast<uint8_t>((value >> 8) & 0xFF);
+    }
+
+    constexpr void write_dword(uint8_t* dest, size_t& offset, uint32_t value)
+    {
+        dest[offset++] = static_cast<uint8_t>(value & 0xFF);
+        dest[offset++] = static_cast<uint8_t>((value >> 8) & 0xFF);
+        dest[offset++] = static_cast<uint8_t>((value >> 16) & 0xFF);
+        dest[offset++] = static_cast<uint8_t>((value >> 24) & 0xFF);
+    }


template <typename T> constexpr void write_integral(uint8_t* dest, size_t& offset, T value) { for (size_t i = 0; i < sizeof(T); ++i) { dest[offset++] = static_cast<uint8_t>((value >> (i * 8)) & 0xFF); } }

dunhor · 2026-04-14T19:01:50Z

+    template <size_t N>
+    constexpr size_t sid_byte_size(const static_sid_t<N>&)
+    {
+        return 8 + 4 * N;


Comment would be nice. Changing to use sizeof would be nicer. Moving this to be a static data member of static_sid_t would be nicest.

dunhor · 2026-04-14T19:19:30Z

+    // A typed ACE containing the ACE header fields and an embedded SID.
+    // Layout mirrors ACCESS_ALLOWED_ACE / ACCESS_DENIED_ACE (same binary layout).
+    template <size_t SubAuthorityCount>
+    struct static_ace_t
+    {
+        uint8_t AceType;
+        uint8_t AceFlags;
+        uint32_t Mask;
+        static_sid_t<SubAuthorityCount> Sid;


Both ACCESS_ALLOWED_ACE and ACCESS_DENIED_ACE are defined like:

typedef struct _ACCESS_*_ACE { ACE_HEADER Header; ACCESS_MASK Mask; DWORD SidStart; } ACCESS_*_ACE;

and ACE_HEADER is defined as:

typedef struct _ACE_HEADER { BYTE AceType; BYTE AceFlags; WORD AceSize; } ACE_HEADER;

This seems to be missing ~~both~~ AceSize ~~and SidStart~~, so at least on first glance, this doesn't appear to be the same binary layout as those types. Am I missing something here?

~~Curiously the byte_size function/comment mention AceSize, however it doesn't make mention of SidStart.~~ Ahh, okay, I neglected to read the purpose of SidStart - not relevant here

I see also that write_ace accounts for AceSize, so perhaps the comment is just misleading - this isn't the same binary layout?

dunhor · 2026-04-14T19:23:13Z

+        static constexpr size_t byte_size()
+        {
+            // ACE_HEADER (Type:1 + Flags:1 + Size:2) + Mask (4) + SID (8 + 4*N)
+            return 4 + 4 + 8 + 4 * SubAuthorityCount;
+        }


Another that's probably better as a static data member

dunhor · 2026-04-14T20:55:51Z

+        bool seenAllow = false;
+        for (size_t i = 0; i < count; ++i)
+        {
+            if (types[i] == ACCESS_ALLOWED_ACE_TYPE)
+            {
+                seenAllow = true;
+            }
+            else if (types[i] == ACCESS_DENIED_ACE_TYPE && seenAllow)
+            {
+                return false; // deny after allow
+            }
+        }


Maybe I'm weird, but two loops always seem clearer to me

size_t i = 0; for (; i < count; ++i) { if (types[i] == ACCESS_ALLOWED_ACE_TYPE) { ++i; break; } } for (; i < count; ++i) { if (types[i] != ACCESS_ALLOWED_ACE_TYPE) { return false; } } return true;

Or if STL use is okay here

auto begin = types; auto end = types + count; return std::find(std::find(begin, end, ACCESS_ALLOWED_ACE_TYPE), end, ACCESS_DENIED_ACE_TYPE) == end;

dunhor · 2026-04-14T21:35:28Z

+
+        PSECURITY_DESCRIPTOR get() const
+        {
+            return reinterpret_cast<PSECURITY_DESCRIPTOR>(const_cast<uint8_t*>(data));


I get that the const_cast is because PSECURITY_DESCRIPTOR is PVOID and there's no PCSECURITY_DESCRIPTOR, but probably worth a comment

dunhor · 2026-04-14T21:37:50Z

+template <typename OwnerSid, typename GroupSid, typename... Aces>
+constexpr auto make_self_relative_sd(const OwnerSid& owner, const GroupSid& group, const Aces&... aces)


What's the purpose of having type safe sd_owner_t and sd_group_t types if the inputs are just templates anyway and AFAICT there's no enforcement anywhere?

dunhor · 2026-04-14T21:46:46Z

+    constexpr size_t acesSize = details::total_ace_size<Aces...>();
+    constexpr size_t totalSize = sdHeaderSize + ownerSize + groupSize + aclHeaderSize + acesSize;
+
+    // Deny ACEs must precede allow ACEs — will fail constexpr evaluation if violated.


I'm actually surprised WI_ASSERT is allowed in constexpr contexts. I actually had issues with this recently when compiling code that uses NT_ASSERT with Clang and IIRC WI_ASSERT is basically a re-packaging of NT_ASSERT.

But let's just assume that it's fine (since MSVC doesn't seem to complain about it) - this would only fail compilation when compiling for Debug, right?

FWIW throwing is the reliable way to fail compilation. You can wrap with if (std::is_constant_evaluated()) (C++20 - __cpp_lib_is_constant_evaluated >= 201811L && <type_traits>) or if consteval (C++23) to avoid the check at runtime if desired

jonwis added 6 commits April 1, 2026 15:15

Static SDDL processing

8d47f13

Add specific types to show owner vs group

49cecae

Now with static strings

421ffee

Merge branch 'master' into user/jonwis/static-sddl

544d027

dmachaj reviewed Apr 2, 2026

View reviewed changes

dunhor reviewed Apr 2, 2026

View reviewed changes

dunhor reviewed Apr 14, 2026

View reviewed changes

Merge branch 'master' into user/jonwis/static-sddl

1605f21

		template <typename OwnerSid, typename GroupSid, typename... Aces>
		constexpr auto make_self_relative_sd(const OwnerSid& owner, const GroupSid& group, const Aces&... aces)

Conversation

jonwis commented Apr 1, 2026

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!

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!

dunhor Apr 14, 2026 • 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!

dunhor Apr 14, 2026 • 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!

dunhor Apr 14, 2026 • 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!

dunhor Apr 14, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dunhor Apr 14, 2026 •

edited

Loading

dunhor Apr 14, 2026 •

edited

Loading

dunhor Apr 14, 2026 •

edited

Loading

dunhor Apr 14, 2026 •

edited

Loading