Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VAULT-8703 Add warning for dangerous undocumented overrides, if used, in status response #17855

Merged
merged 3 commits into from Nov 9, 2022
Merged

VAULT-8703 Add warning for dangerous undocumented overrides, if used, in status response #17855

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9839ba
VAULT-8703 Add warning for dangerous undocumented overrides, if used,…
VioletHynes Nov 8, 2022
9964ed5
VAULT-8703 add changelog
VioletHynes Nov 8, 2022
261c179
VAULT-8703 fix append
VioletHynes Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 17 additions & 16 deletions api/sys_seal.go
View file
Open in desktop
Expand Up @@ -93,22 +93,23 @@ func sealStatusRequestWithContext(ctx context.Context, c *Sys, r *Request) (*Sea
}

type SealStatusResponse struct {
Type string `json:"type"`
Initialized bool `json:"initialized"`
Sealed bool `json:"sealed"`
T int `json:"t"`
N int `json:"n"`
Progress int `json:"progress"`
Nonce string `json:"nonce"`
Version string `json:"version"`
BuildDate string `json:"build_date"`
Migration bool `json:"migration"`
ClusterName string `json:"cluster_name,omitempty"`
ClusterID string `json:"cluster_id,omitempty"`
RecoverySeal bool `json:"recovery_seal"`
StorageType string `json:"storage_type,omitempty"`
HCPLinkStatus string `json:"hcp_link_status,omitempty"`
HCPLinkResourceID string `json:"hcp_link_resource_ID,omitempty"`
Type string `json:"type"`
Initialized bool `json:"initialized"`
Sealed bool `json:"sealed"`
T int `json:"t"`
N int `json:"n"`
Progress int `json:"progress"`
Nonce string `json:"nonce"`
Version string `json:"version"`
BuildDate string `json:"build_date"`
Migration bool `json:"migration"`
ClusterName string `json:"cluster_name,omitempty"`
ClusterID string `json:"cluster_id,omitempty"`
RecoverySeal bool `json:"recovery_seal"`
StorageType string `json:"storage_type,omitempty"`
HCPLinkStatus string `json:"hcp_link_status,omitempty"`
HCPLinkResourceID string `json:"hcp_link_resource_ID,omitempty"`
Warnings []string `json:"warnings,omitempty"`
}

type UnsealOpts struct {
Expand Down
3 changes: 3 additions & 0 deletions changelog/17855.txt
View file
Open in desktop
@@ -0,0 +1,3 @@
```release-note:improvement
core: Added warning to /sys/seal-status and vault status command if potentially dangerous behaviour overrides are being used.
```
3 changes: 3 additions & 0 deletions command/format.go
View file
Open in desktop
Expand Up @@ -402,6 +402,9 @@ func (t TableFormatter) OutputSealStatusStruct(ui cli.Ui, secret *api.Secret, da
if status.LastWAL != 0 {
out = append(out, fmt.Sprintf("Last WAL | %d", status.LastWAL))
}
if len(status.Warnings) > 0 {
out = append(out, fmt.Sprintf("Warnings | %v", status.Warnings))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect %v isn't going to do a great job of handling multiple strings containing whitespace, it won't be clear where one ends and the next begins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it does kinda just append them. I think to some extent that should be on us to word them well, but similarly, I would hope we don't get into the kind of state where someone has 10 of these kinds of warnings.

In other words, this could be optimistic, but I feel like >1 warning is a very rare case, as long as we keep what we warn on broadly consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might look funny but would it be worth quoting them so there is at least some means of separation?

}

ui.Output(tableOutput(out, &columnize.Config{
Delim: "|",
Expand Down
4 changes: 3 additions & 1 deletion command/format_test.go
View file
Open in desktop
Expand Up @@ -118,7 +118,8 @@ Cluster ID cluster id
HA Enabled true
Raft Committed Index 3
Raft Applied Index 4
Last WAL 2`
Last WAL 2
Warnings [warning]`

if expectedOutputString != output {
fmt.Printf("%s\n%+v\n %s\n%+v\n", "output found was: ", output, "versus", expectedOutputString)
Expand Down Expand Up @@ -174,6 +175,7 @@ func getMockStatusData(emptyFields bool) SealStatusOutput {
ClusterID: "cluster id",
RecoverySeal: true,
StorageType: "storage type",
Warnings: append(make([]string, 0), "warning"),
VioletHynes marked this conversation as resolved.
Show resolved Hide resolved
}

// must initialize this struct without explicit field names due to embedding
Expand Down
74 changes: 74 additions & 0 deletions http/sys_seal_test.go
View file
Open in desktop
Expand Up @@ -60,6 +60,80 @@ func TestSysSealStatus(t *testing.T) {
}
}

func TestSysSealStatus_Warnings(t *testing.T) {
core := vault.TestCore(t)
vault.TestCoreInit(t, core)
ln, addr := TestServer(t, core)
defer ln.Close()

// Manually configure DisableSSCTokens to be true
core.GetCoreConfigInternal().DisableSSCTokens = true

resp, err := http.Get(addr + "/v1/sys/seal-status")
if err != nil {
t.Fatalf("err: %s", err)
}

var actual map[string]interface{}
expected := map[string]interface{}{
"sealed": true,
"t": json.Number("3"),
"n": json.Number("3"),
"progress": json.Number("0"),
"nonce": "",
"type": "shamir",
"recovery_seal": false,
"initialized": true,
"migration": false,
"build_date": version.BuildDate,
}
testResponseStatus(t, resp, 200)
testResponseBody(t, resp, &actual)
if actual["version"] == nil {
t.Fatalf("expected version information")
}
expected["version"] = actual["version"]
if actual["cluster_name"] == nil {
delete(expected, "cluster_name")
} else {
expected["cluster_name"] = actual["cluster_name"]
}
if actual["cluster_id"] == nil {
delete(expected, "cluster_id")
} else {
expected["cluster_id"] = actual["cluster_id"]
}
actualWarnings := actual["warnings"]
if actualWarnings == nil {
t.Fatalf("expected warnings about SSCToken disabling")
}

actualWarningsArray, ok := actualWarnings.([]interface{})
if !ok {
t.Fatalf("expected warnings about SSCToken disabling were not in the right format")
}
if len(actualWarningsArray) != 1 {
t.Fatalf("too many warnings were given")
}
actualWarning, ok := actualWarningsArray[0].(string)
if !ok {
t.Fatalf("expected warning about SSCToken disabling was not in the right format")
}

expectedWarning := "Server Side Consistent Tokens are disabled, due to the " +
"VAULT_DISABLE_SERVER_SIDE_CONSISTENT_TOKENS environment variable being set. " +
"It is not recommended to run Vault for an extended period of time with this configuration."
if actualWarning != expectedWarning {
t.Fatalf("actual warning was not as expected. Expected %s, but got %s", expectedWarning, actualWarning)
}

expected["warnings"] = actual["warnings"]

if diff := deep.Equal(actual, expected); diff != nil {
t.Fatal(diff)
}
}

func TestSysSealStatus_uninit(t *testing.T) {
core := vault.TestCore(t)
ln, addr := TestServer(t, core)
Expand Down
47 changes: 31 additions & 16 deletions vault/logical_system.go
View file
Open in desktop
Expand Up @@ -4393,22 +4393,36 @@ func (b *SystemBackend) pathInternalOpenAPI(ctx context.Context, req *logical.Re
}

type SealStatusResponse struct {
Type string `json:"type"`
Initialized bool `json:"initialized"`
Sealed bool `json:"sealed"`
T int `json:"t"`
N int `json:"n"`
Progress int `json:"progress"`
Nonce string `json:"nonce"`
Version string `json:"version"`
BuildDate string `json:"build_date"`
Migration bool `json:"migration"`
ClusterName string `json:"cluster_name,omitempty"`
ClusterID string `json:"cluster_id,omitempty"`
RecoverySeal bool `json:"recovery_seal"`
StorageType string `json:"storage_type,omitempty"`
HCPLinkStatus string `json:"hcp_link_status,omitempty"`
HCPLinkResourceID string `json:"hcp_link_resource_ID,omitempty"`
Type string `json:"type"`
Initialized bool `json:"initialized"`
Sealed bool `json:"sealed"`
T int `json:"t"`
N int `json:"n"`
Progress int `json:"progress"`
Nonce string `json:"nonce"`
Version string `json:"version"`
BuildDate string `json:"build_date"`
Migration bool `json:"migration"`
ClusterName string `json:"cluster_name,omitempty"`
ClusterID string `json:"cluster_id,omitempty"`
RecoverySeal bool `json:"recovery_seal"`
StorageType string `json:"storage_type,omitempty"`
HCPLinkStatus string `json:"hcp_link_status,omitempty"`
HCPLinkResourceID string `json:"hcp_link_resource_ID,omitempty"`
Warnings []string `json:"warnings,omitempty"`
}

// getStatusWarnings exposes potentially dangerous overrides in the status response
// currently, this only warns about VAULT_DISABLE_SERVER_SIDE_CONSISTENT_TOKENS,
// but should be extended to report more warnings where appropriate
func (core *Core) getStatusWarnings() []string {
var warnings []string
if core.GetCoreConfigInternal() != nil && core.GetCoreConfigInternal().DisableSSCTokens {
warnings = append(warnings, "Server Side Consistent Tokens are disabled, due to the "+
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we have a policy that we introduce warnings like this, we link them to something external that we can modify? I just worry that otherwise we may have to come back multiple times to this code to add extra details in response to user questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on what you mean, this could amount to us having some level of documentation for these (otherwise) undocumented options, which does have potential implications. I'm not entirely against this, but I worry it would go against our core goal for the kinds of options we want to warn about.

@ccapurso can probably confirm in more detail, but I think the main onus for this is some kind of way for us to tell at-a-glance that this feature is being used, as well as reminding the customer that it's not particularly supported. I hope we wouldn't require too much detail here, and I hope the number of customers that see these warnings remains extremely low.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding of the original discussion was that these warnings were going to identify that exceptional overrides were being used. Long-lived, documented overrides are often less likely to be harmful. The means to identify something in this way is almost more for identifying potential things to look at during incident response.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this could amount to us having some level of documentation for these (otherwise) undocumented options

Forgot about that wrinkle.

If you went with something shorter like "VAULT_DISABLE_SERVER_SIDE_CONSISTENT_TOKENS=true", that would address my other comment about %v, and this one too, in the sense that I'm not worried that we'll have to revisit the verbiage. It wouldn't do a great job of "reminding the customer that it's not particularly supported", but I guess I'm afraid that we're biting off more than we can chew in attempting to pithily tell the user they should be moving away from this setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, we wouldn't necessarily be giving warnings -- which might be okay, to be clear. If we shorten it, we'd need a different name, like "Unrecommended Settings" (open to suggestions if you can think of something that sounds better), but I guess my worry with that is that that's also lacking in description too. I guess in either case, we're picking our poison.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm not hung up on this, I'm going to approve and leave it for you to decide. My concerns may be unwarranted, I just imagine various scenarios where we get into trouble by trying to thread this needle of giving vague warnings without expanding on them anywhere. Like what if a big company using the var tries to upgrade, sees this, and tells us: "Look, we know we need to move away from this, and we're working on it, but our users will freak out if they see this message, can you tone it down? We can't upgrade with this text in vault status."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I definitely see the argument. Like I said, I guess I see this as picking our poison, as something like "Unsupported Settings" could make customers raise their hackles too.

I think I'll keep it as-is, especially as most customers hopefully won't see this, and "Warnings" feels like quite a generic name if we do want to change the contents in the future.

"VAULT_DISABLE_SERVER_SIDE_CONSISTENT_TOKENS environment variable being set. "+
"It is not recommended to run Vault for an extended period of time with this configuration.")
}
return warnings
}

func (core *Core) GetSealStatus(ctx context.Context) (*SealStatusResponse, error) {
Expand Down Expand Up @@ -4481,6 +4495,7 @@ func (core *Core) GetSealStatus(ctx context.Context) (*SealStatusResponse, error
ClusterID: clusterID,
RecoverySeal: core.SealAccess().RecoveryKeySupported(),
StorageType: core.StorageType(),
Warnings: core.getStatusWarnings(),
}

if resourceIDonHCP != "" {
Expand Down