fix: Fix msgpack decode and auth-method encoding bugs#895
Merged
Conversation
|
🦢 Load Test Results Goose Attack ReportPlan Overview
Request Metrics
Response Time Metrics
Status Code Metrics
Transaction Metrics
Scenario Metrics
|
|
| Branch | claude/review-token-driver-fernet-21ssvf |
| Testbed | ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| Command_Serde/apply/remove | 📈 view plot 🚷 view threshold | 218,270.00 ns(-21.41%)Baseline: 277,737.50 ns | 1,683,725.46 ns (12.96%) |
| Command_Serde/apply/set | 📈 view plot 🚷 view threshold | 209,560.00 ns(-7.91%)Baseline: 227,551.36 ns | 980,590.97 ns (21.37%) |
| Command_Serde/pack/delete | 📈 view plot 🚷 view threshold | 118.93 ns(-1.30%)Baseline: 120.49 ns | 140.35 ns (84.74%) |
| Command_Serde/pack/delete_index | 📈 view plot 🚷 view threshold | 109.05 ns(-0.84%)Baseline: 109.97 ns | 127.97 ns (85.22%) |
| Command_Serde/pack/set | 📈 view plot 🚷 view threshold | 191.30 ns(-1.01%)Baseline: 193.25 ns | 228.86 ns (83.59%) |
| Command_Serde/pack/set_index | 📈 view plot 🚷 view threshold | 108.85 ns(-1.04%)Baseline: 109.99 ns | 127.91 ns (85.10%) |
| Command_Serde/unpack/delete | 📈 view plot 🚷 view threshold | 199.03 ns(+2.91%)Baseline: 193.40 ns | 234.43 ns (84.90%) |
| Command_Serde/unpack/delete_index | 📈 view plot 🚷 view threshold | 162.56 ns(+0.50%)Baseline: 161.75 ns | 198.41 ns (81.93%) |
| Command_Serde/unpack/set | 📈 view plot 🚷 view threshold | 272.64 ns(+4.15%)Baseline: 261.76 ns | 321.95 ns (84.68%) |
| Command_Serde/unpack/set_index | 📈 view plot 🚷 view threshold | 160.69 ns(+0.30%)Baseline: 160.21 ns | 195.58 ns (82.16%) |
| Payload_encryption/pack/remove_cmd | 📈 view plot 🚷 view threshold | 116.94 ns(-1.05%)Baseline: 118.18 ns | 150.85 ns (77.52%) |
| Payload_encryption/pack/set_cmd | 📈 view plot 🚷 view threshold | 191.72 ns(-6.96%)Baseline: 206.06 ns | 269.63 ns (71.11%) |
| Payload_encryption/unpack/remove_cmd | 📈 view plot 🚷 view threshold | 213.84 ns(+4.19%)Baseline: 205.24 ns | 246.43 ns (86.78%) |
| Payload_encryption/unpack/set_cmd | 📈 view plot 🚷 view threshold | 286.12 ns(+4.56%)Baseline: 273.63 ns | 336.40 ns (85.05%) |
| Raft_1Node_Latency/prefix/1node | 📈 view plot 🚷 view threshold | 1,929,400.00 ns(-33.47%)Baseline: 2,899,955.78 ns | 5,886,565.94 ns (32.78%) |
| Raft_1Node_Latency/read/1node | 📈 view plot 🚷 view threshold | 39,507.00 ns(+534.13%)Baseline: 6,230.07 ns | 40,011.94 ns (98.74%) |
| Raft_1Node_Latency/remove/1node | 📈 view plot 🚷 view threshold | 476,860.00 ns(-10.94%)Baseline: 535,433.75 ns | 2,288,831.35 ns (20.83%) |
| Raft_1Node_Latency/write/1node | 📈 view plot 🚷 view threshold | 533,330.00 ns(-3.76%)Baseline: 554,192.03 ns | 2,122,030.61 ns (25.13%) |
| build_snapshot/default | 📈 view plot 🚷 view threshold | 102,450.00 ns(-1.13%)Baseline: 103,618.45 ns | 162,411.12 ns (63.08%) |
| fernet token/project | 📈 view plot 🚷 view threshold | 1,403.40 ns(+0.77%)Baseline: 1,392.64 ns | 1,616.66 ns (86.81%) |
| get_data_keyspace | 📈 view plot 🚷 view threshold | 0.31 ns(-1.69%)Baseline: 0.32 ns | 0.37 ns (83.99%) |
| get_db | 📈 view plot 🚷 view threshold | 0.31 ns(-1.44%)Baseline: 0.32 ns | 0.37 ns (84.32%) |
| get_fernet_token_timestamp/project | 📈 view plot 🚷 view threshold | 144.98 ns(-0.51%)Baseline: 145.72 ns | 179.69 ns (80.68%) |
| get_keyspace | 📈 view plot 🚷 view threshold | 4.36 ns(-7.90%)Baseline: 4.74 ns | 8.76 ns (49.81%) |
Fixes several correctness/security bugs found while reviewing the Fernet token driver: - read_str/read_uuid/read_audit_ids used read_pfix (a marker-aware positive-fixint reader) to read the raw length byte that follows a Bin8 marker, and read_str didn't handle the Str8/Str16/Str32 markers that write_str actually emits for strings >=32 bytes. Any non-UUID identifier >=128 bytes or string field >=32 bytes (system_id, protocol_id, long federated user/group ids, etc.) failed to decode. Both readers now correctly parse the raw (non-MessagePack) length that follows Bin8/16/32 and Str8/16/32 markers. - The auth-methods bitmask byte was written with write_pfix, which asserts val < 128 and panics for any combination using the 8th configured auth method (bit 128). Added write_auth_methods_code / read_auth_methods_code helpers that use a positive fixint for values <128 (unchanged wire format, still fully backward compatible) and a MessagePack uint8 for values >=128, and switched all ten token payload modules to use them. - FernetTokenProvider::reload_config computed 1u8 << k for each configured auth method by index; with more than 8 methods configured this shift-overflows (panics in debug, silently collides bits in release). Now caps the bitmask to the first 8 methods and warns if more are configured. - FernetTokenProvider::decode_auth_methods's cache-miss fallback walked the bit->name map in ascending order, but the "auth / idx == 1" bit test is only valid from the largest bit down to the smallest; ascending order silently dropped smaller bits whenever a larger bit was also set, under-reporting the auth methods used for a token. Fixed to iterate in descending order. - FernetUtils::create_tmp_new_key installed the scopeguard that restores the original euid/egid only after both setegid and seteuid succeeded, so a seteuid failure after a successful setegid left the process with its effective group permanently changed. The guard is now installed right after setegid succeeds, before seteuid is attempted. Added regression tests for each of the above. Signed-off-by: Artem Goncharov <artem.goncharov@gmail.com>
f52e7b3 to
9402b7a
Compare
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes several correctness/security bugs found while reviewing the Fernet
token driver:
read_str/read_uuid/read_audit_ids used read_pfix (a marker-aware
positive-fixint reader) to read the raw length byte that follows a
Bin8 marker, and read_str didn't handle the Str8/Str16/Str32 markers
that write_str actually emits for strings >=32 bytes. Any non-UUID
identifier >=128 bytes or string field >=32 bytes (system_id,
protocol_id, long federated user/group ids, etc.) failed to decode.
Both readers now correctly parse the raw (non-MessagePack) length that
follows Bin8/16/32 and Str8/16/32 markers.
The auth-methods bitmask byte was written with write_pfix, which
asserts val < 128 and panics for any combination using the 8th
configured auth method (bit 128). Added write_auth_methods_code /
read_auth_methods_code helpers that use a positive fixint for values
<128 (unchanged wire format, still fully backward compatible) and a
MessagePack uint8 for values >=128, and switched all ten token payload
modules to use them.
FernetTokenProvider::reload_config computed 1u8 << k for each
configured auth method by index; with more than 8 methods configured
this shift-overflows (panics in debug, silently collides bits in
release). Now caps the bitmask to the first 8 methods and warns if
more are configured.
FernetTokenProvider::decode_auth_methods's cache-miss fallback walked
the bit->name map in ascending order, but the "auth / idx == 1" bit
test is only valid from the largest bit down to the smallest;
ascending order silently dropped smaller bits whenever a larger bit
was also set, under-reporting the auth methods used for a token.
Fixed to iterate in descending order.
FernetUtils::create_tmp_new_key installed the scopeguard that restores
the original euid/egid only after both setegid and seteuid succeeded,
so a seteuid failure after a successful setegid left the process with
its effective group permanently changed. The guard is now installed
right after setegid succeeds, before seteuid is attempted.
Added regression tests for each of the above.
Signed-off-by: Artem Goncharov artem.goncharov@gmail.com