replicate the auth.db file to secondaries - #3057
Open
zachmu wants to merge 5 commits into
Open
Conversation
Contributor
|
|
SummaryThe run broadly covers authentication and authorization lifecycle behavior, including role and privilege changes, persistence, replication, startup, and failover, with both normal user flows and adversarial cases such as unauthorized changes and malformed state. It also exercises edge cases around expiry handling and preserving authorization state across distributed transitions. Not safe to merge yet — a PR-attributable malformed authorization-state case can crash the server, making this a high-severity availability and access-control risk at the replication boundary. An unrelated medium-severity expiry-date issue remains as a flag for later, but is not a merge driver. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Valid role expiry dates are rejected
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
| return nil | ||
| } | ||
|
|
||
| // OverwriteDatabase replaces the entire contents of the auth database, both in memory and on disk, with the |
There was a problem hiding this comment.
Malformed auth data can crash a standby
What failed: The standby does not safely reject every malformed supported-version payload. Short payloads and unknown versions return errors, but a truncated version 0 or version 1 payload can make the server panic during decoding.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: A malformed authorization update can crash the server instead of being rejected. Users may lose access to protected services until the server recovers.
- Steps to Reproduce:
- Send a payload whose first four bytes identify version 0 or version 1 but truncate the remaining serialized fields.
- Apply the payload through the standby authorization-state replacement operation.
- Observe that decoding can panic instead of returning an error; the expected behavior is to reject the payload and leave the prior authorization state unchanged.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR adds OverwriteDatabase in server/auth/cluster.go:70-83. It creates a fresh database, calls fresh.deserialize(data) at lines 75-78, and swaps globalDatabase only after that call succeeds at line 80, which correctly protects the old state from ordinary returned errors but does not protect the process from a panic. The PR also changes server/auth/serialization.go:68-81 to dispatch version 0 and version 1 payloads through deserializeV0 and deserializeV1. Those loaders call Role.deserialize and the privilege deserializers without validating that all encoded fields are present. The underlying reader is unchecked: utils/reader.go:70-73 indexes buf directly for Uint8, lines 76-90 slices the buffer for fixed-width integers, lines 129-192 indexes ahead while decoding variable integers, and lines 195-200 advances by the encoded string length before slicing. A supported-version payload truncated at any of these operations therefore raises a Go bounds panic rather than producing an error for OverwriteDatabase to return. The smallest practical mitigation is to make the overwrite boundary convert malformed decoding panics into errors, while a stronger targeted fix is to add bounds checks to the reader methods used by auth deserialization and propagate those errors. Either approach must keep the global state swap after successful decoding only.
- Why this is likely a bug: The required contract is to reject malformed supported-version data with an error and preserve the old authorization state. The source path confirms the opposite failure mode: version 0/1 decoding uses unchecked reads, so truncated input can escape the normal error return and panic the process. This is a real production risk at the new replication boundary even though the runtime server was unavailable and the available integration test covers only valid replication. The smallest fix is to guard this boundary or, preferably, return bounds errors from the reader operations used by auth decoding; do not assign globalDatabase or write the malformed bytes unless decoding completes successfully.
Relevant code
server/auth/cluster.go:70-83
func OverwriteDatabase(data []byte) error {
var err error
LockWrite(func() {
fresh := newEmptyDatabase()
if err = fresh.deserialize(data); err != nil {
return
}
globalDatabase = fresh
err = WriteSerializedDatabase(data)
})
return err
}server/auth/serialization.go:64-81
func (db *Database) deserialize(data []byte) error {
if len(data) < 4 {
return errors.New("invalid auth database format")
}
reader := utils.NewReader(data)
version := reader.Uint32()
var err error
switch version {
case 0:
err = db.deserializeV0(reader)
case 1:
err = db.deserializeV1(reader)
default:
return errors.Errorf("Authorization database format %d is not supported, please upgrade Doltgres", version)
}
if err != nil {
return err
}utils/reader.go:69-90
func (reader *Reader) Uint8() uint8 {
reader.offset += 1
return reader.buf[reader.offset-1]
}
func (reader *Reader) Uint16() uint16 {
reader.offset += 2
return binary.BigEndian.Uint16(reader.buf[reader.offset-2:])
}
func (reader *Reader) Uint32() uint32 {
reader.offset += 4
return binary.BigEndian.Uint32(reader.buf[reader.offset-4:])
}
func (reader *Reader) Uint64() uint64 {
reader.offset += 8
return binary.BigEndian.Uint64(reader.buf[reader.offset-8:])
}utils/reader.go:195-200
func (reader *Reader) String() string {
length := reader.VariableUint()
reader.offset += length
return string(reader.buf[reader.offset-length : reader.offset])
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Malformed auth data can crash a standby**
**What failed:** The standby does not safely reject every malformed supported-version payload. Short payloads and unknown versions return errors, but a truncated version 0 or version 1 payload can make the server panic during decoding.
- **Impact:** A malformed authorization update can crash the server instead of being rejected. Users may lose access to protected services until the server recovers.
- **Steps to reproduce:**
1. Send a payload whose first four bytes identify version 0 or version 1 but truncate the remaining serialized fields.
2. Apply the payload through the standby authorization-state replacement operation.
3. Observe that decoding can panic instead of returning an error; the expected behavior is to reject the payload and leave the prior authorization state unchanged.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR adds OverwriteDatabase in server/auth/cluster.go:70-83. It creates a fresh database, calls fresh.deserialize(data) at lines 75-78, and swaps globalDatabase only after that call succeeds at line 80, which correctly protects the old state from ordinary returned errors but does not protect the process from a panic. The PR also changes server/auth/serialization.go:68-81 to dispatch version 0 and version 1 payloads through deserializeV0 and deserializeV1. Those loaders call Role.deserialize and the privilege deserializers without validating that all encoded fields are present. The underlying reader is unchecked: utils/reader.go:70-73 indexes buf directly for Uint8, lines 76-90 slices the buffer for fixed-width integers, lines 129-192 indexes ahead while decoding variable integers, and lines 195-200 advances by the encoded string length before slicing. A supported-version payload truncated at any of these operations therefore raises a Go bounds panic rather than producing an error for OverwriteDatabase to return. The smallest practical mitigation is to make the overwrite boundary convert malformed decoding panics into errors, while a stronger targeted fix is to add bounds checks to the reader methods used by auth deserialization and propagate those errors. Either approach must keep the global state swap after successful decoding only.
- **Why this is likely a bug:** The required contract is to reject malformed supported-version data with an error and preserve the old authorization state. The source path confirms the opposite failure mode: version 0/1 decoding uses unchecked reads, so truncated input can escape the normal error return and panic the process. This is a real production risk at the new replication boundary even though the runtime server was unavailable and the available integration test covers only valid replication. The smallest fix is to guard this boundary or, preferably, return bounds errors from the reader operations used by auth decoding; do not assign globalDatabase or write the malformed bytes unless decoding completes successfully.
**Relevant code:**
`server/auth/cluster.go:70-83`
~~~go
func OverwriteDatabase(data []byte) error {
var err error
LockWrite(func() {
fresh := newEmptyDatabase()
if err = fresh.deserialize(data); err != nil {
return
}
globalDatabase = fresh
err = WriteSerializedDatabase(data)
})
return err
}
~~~
`server/auth/serialization.go:64-81`
~~~go
func (db *Database) deserialize(data []byte) error {
if len(data) < 4 {
return errors.New("invalid auth database format")
}
reader := utils.NewReader(data)
version := reader.Uint32()
var err error
switch version {
case 0:
err = db.deserializeV0(reader)
case 1:
err = db.deserializeV1(reader)
default:
return errors.Errorf("Authorization database format %d is not supported, please upgrade Doltgres", version)
}
if err != nil {
return err
}
~~~
`utils/reader.go:69-90`
~~~go
func (reader *Reader) Uint8() uint8 {
reader.offset += 1
return reader.buf[reader.offset-1]
}
func (reader *Reader) Uint16() uint16 {
reader.offset += 2
return binary.BigEndian.Uint16(reader.buf[reader.offset-2:])
}
func (reader *Reader) Uint32() uint32 {
reader.offset += 4
return binary.BigEndian.Uint32(reader.buf[reader.offset-4:])
}
func (reader *Reader) Uint64() uint64 {
reader.offset += 8
return binary.BigEndian.Uint64(reader.buf[reader.offset-8:])
}
~~~
`utils/reader.go:195-200`
~~~go
func (reader *Reader) String() string {
length := reader.VariableUint()
reader.offset += length
return string(reader.buf[reader.offset-length : reader.offset])
}
~~~
Contributor
|
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
1 participant
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.
This closes a significant gap in replication.