hashicorp / hashicorp/consul

ca: improve the data model

Open
#11,347 0 comments 0 reactions 0 assignees View on GitHub
theme/certificates theme/connect theme/internal-cleanup
Dominant language
Go
Stars
30.1k
Forks
4.6k
Avg merge
1d 18h
Merged PRs (30d)
39

Description

The Connect CA system has three core structs that are used in many places as part of both its data model and RPC request/response.

* `IndexedCARoots` - the TrustDomain/ClusterID, ActiveRootID, and all the `CARoot` (active and not, which also include intermediates and other things, see below)
* `CARoot` - the root certificate, but also any intermediate certs, private and signing key details, root expiry time, `ExternalTrustDomain`.
* `CAConfiguration` - provider name, an opaque map of configuration for the provider, `ClusterID` (which can't be set by the user), `State` (which can only be initialized once)

While working on the CA system I noticed a few problems with this model which seem like they may be prone to cause bugs:

1. we manually reconstruct a `CARoot` to return it to the user, and as part of this we attempt to remove "sensitive" data: https://github.com/hashicorp/consul/blob/v1.10.3/agent/consul/server_connect.go#L102-L135. Since the fields still exist, it seems like it would be pretty easy to leak the wrong data. If we had a response struct with only the relevant fields that would be harder to get wrong.
2. The `ClusterID` is not actually settable by the user, but it appears in the `CAConfiguration` struct that is submitted by the user https://github.com/hashicorp/consul/blob/v1.10.3/agent/consul/leader_connect_ca.go#L756-L757.
3. In some places we call it `ClusterID`, and in others we call it `TrustDomain` (or `ExternalTrustDomain`), and yet these are all the same value.
4. `CAConfiguration.State` is particularly strange. It's part of the config, but in practice is never set by the user, and it's actually an error for the user to change its value. The value comes from `Provider.State`. But then there is also a `CAConsulProviderState` table that one provider uses, which can also be used for state. Instead of persisting the state in `CaConfiguration` and having a bunch of logic to prevent updates to it, we should have the providers manage their own state like the Consul provider does. Only the AWS provider uses this, so it should be easy to remove.

(additional issues may be added to this list in the future)

I believe we could address many of these problems by:
1. use separate types for request/response and memdb storage. That way each type will only include the fields that are relevant for how the type is being used.
2. normalize the names of fields so they are consistent
3. we know the structure of the opaque config will include `CommonCAProviderConfig` and one of the provider config structs. We could parse this into a normalized structure earlier instead of dealing with opaque configuration in multiple places.

Contributor guide

Open the contributing guide

Research direction

Start by reading the CA handling in agent/consul/server_connect.go and agent/consul/leader_connect_ca.go, focusing on CARoot and CAConfiguration construction and validation. Compare those uses with IndexedCARoots, provider state, and the CAConsulProviderState table. Done requires an agreed, consistent request/response/storage model and removal of the identified state and naming ambiguities.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.