Default crl expiry

[JNProtzman]

Ref: #17642

[hashicorp-cla]

All committers have signed the CLA.

[JNProtzman]

Please let me know if I should authorize Vercel. Also, it looks like test-go-race is failing on master but with different tests?

[JNProtzman]

I was looking through the tests and I think the root issue is already resolved here.

I found a regression test for a similar issue.

I will do some additional testing on a local Vault instance tomorrow.

[cipherboy]

This wouldn't fix the issue, right? It happens in the same control flow as the earlier code change, just in a different order.

I believe the solution I was suggesting was to add it here, which you've done below:

vault/builtin/logical/pki/storage.go

Lines 1152 to 1178 in 0423ffb

	func (sc *storageContext) getRevocationConfig() (*crlConfig, error) {
	entry, err := sc.Storage.Get(sc.Context, "config/crl")
	if err != nil {
	return nil, err
	}
	var result crlConfig
	if entry == nil {
	result = defaultCrlConfig
	return &result, nil
	}
	if err = entry.DecodeJSON(&result); err != nil {
	return nil, err
	}
	if result.Version == 0 {
	// Automatically update existing configurations.
	result.OcspDisable = defaultCrlConfig.OcspDisable
	result.OcspExpiry = defaultCrlConfig.OcspExpiry
	result.AutoRebuild = defaultCrlConfig.AutoRebuild
	result.AutoRebuildGracePeriod = defaultCrlConfig.AutoRebuildGracePeriod
	result.Version = 1
	}
	return &result, nil
	}

In particular, the bug is that previous versions of Vault allowed empty strings (if the CRL config was never written), but we now strictly enforce not having that.

So we should replace those empty strings with the default values.

In particular the test you point out here:

vault/builtin/logical/pki/crl_test.go

Lines 948 to 960 in 0423ffb

	// Regression test: ensure we respond with the default values for CRL
	// config when we haven't set any values yet.
	resp, err = client.Logical().Read("pki/config/crl")
	require.NoError(t, err)
	require.NotNil(t, resp)
	require.NotNil(t, resp.Data)
	require.Equal(t, resp.Data["expiry"], defaultCrlConfig.Expiry)
	require.Equal(t, resp.Data["disable"], defaultCrlConfig.Disable)
	require.Equal(t, resp.Data["ocsp_disable"], defaultCrlConfig.OcspDisable)
	require.Equal(t, resp.Data["auto_rebuild"], defaultCrlConfig.AutoRebuild)
	require.Equal(t, resp.Data["auto_rebuild_grace_period"], defaultCrlConfig.AutoRebuildGracePeriod)
	require.Equal(t, resp.Data["enable_delta"], defaultCrlConfig.EnableDelta)
	require.Equal(t, resp.Data["delta_rebuild_interval"], defaultCrlConfig.DeltaRebuildInterval)

works, as it should, but only when a Vault 1.12 is used to create the storage entry. If Vault say, 1.9 was used and later upgraded to 1.12, this test would fail, because the 1.9 entry wouldn't have default values added where empty strings were.

I'd prefer to back this particular change out, but leave the storage.go change you have.

[JNProtzman]

I'll go ahead and back this part of the change out, but I do believe it would have resolved the issue (had we not also returned the default when expiry was ""). My reasoning is that the ParseDuration call happens after the check for the CRL being enabled. We have the CRL disabled, and therefore would have returned out of this method before the duration was parsed.

[cipherboy]

I don't think I quite agree.

The modified code removed the goto WRITE; this meant the first if condition would fall through to the code that was before the if, unconditionally, which meant crlInfo.Expiry could still be an empty string in the disabled case with forceNew=true and would still fail the parse. ForceNew is set sometimes, such as generation of a new CA or modification of an existing CA.

If the goto WRITE wasn't intentional and instead was left in and a default value for crlLifetime was chosen (such as 72h), this might've been fine. But I'm not sure its the fix we would've liked (and preferred to fix it read-side so the call is correct). Expiration, IMO, should apply regardless.

Its definitely tricky code though :/ I'd like to simplify it more in the future as I get time.

[JNProtzman]

Setting the default is definitely simpler and has way less implications on the remaining code. I definitely have less experience (a.k.a. no experience) on the codebase. I really appreciate the time you've spent on this (and your thorough explanations), thank you!

[cipherboy]

Yep! This is what I was expecting. The remaining parameters shouldn't have empty string values, as they're all new with 1.12.

[cipherboy]

Thank you for the fix! I'll add a change log and test for this in a follow-up PR. The test gets a little involved as we've gotta write the (pre-1.12) empty config directly to storage and then validate fetching it works as expected, with all defaults.

[JNProtzman]

@cipherboy would you tag me on the PR you put up to add tests? I'd like to see what those look like.