Start counting ACME certificate issuance as client activity #20520

cipherboy · 2023-05-04T19:39:36Z

This enables client count (activity log events) for ACME certificate issuance. Per consensus reached internally, this creates events based upon the unique identifiers requested during issuance. This interface is presently only intended for ACME billing within the builtin PKI plugin, but changes to system view could be made in the future to make this a more generally available interface, including to external plugins. Today, this goes into the non-entity token bucket.

cipherboy · 2023-05-04T19:52:04Z

sdk/logical/acme_billing.go

+
+import "context"
+
+type ACMEBillingSystemView interface {


Not sure if I like this or if we want to rename it now to just BillingSystemView or ActivityEventSystemView or similar.

I do think it's good to make it a bit more general, both of your other suggestions seem good to me

Hmmm... Until we commit to the work in a later release, I'm inclined to leave this as ACME-specific; we don't want others attempting to use this, though admittedly it will only work with builtin plugins for the time being...

stevendpclark

Looks good to me, but I'll let someone from vault-core comment on the system view and activity_log changes.

builtin/logical/pki/acme_billing_test.go

miagilepner · 2023-05-11T13:52:47Z

builtin/logical/pki/acme_billing_hack.go

+
+	var realized []string
+	for _, identifier := range identifiers {
+		realized = append(realized, fmt.Sprintf("%s/%s", identifier.Type, identifier.OriginalValue))


nit: would it be helpful for debugging or anything else to add a method to ACMEIdentifier that returns this formatted string?

I think this is the only place referring to these identifiers in this construction; logging is done below in the system view interface post-conversion, and everywhere else in the API uses JSON or refers by parent identifier value (order ID/authz ID)...

Thoughts @stevendpclark?

builtin/logical/pki/acme_billing_hack.go

miagilepner · 2023-05-11T13:59:07Z

sdk/logical/acme_billing.go

+
+import "context"
+
+type ACMEBillingSystemView interface {


I do think it's good to make it a bit more general, both of your other suggestions seem good to me

vault/acme_billing_system_view.go

builtin/logical/pki/acme_billing_test.go

kitography

looks pretty good :)

kitography · 2023-05-15T10:37:49Z

builtin/logical/pki/acme_billing_test.go

+
+	// Enable additional mounts.
+	setupAcmeBackendOnClusterAtPath(t, cluster, client, "pki2")
+	setupAcmeBackendOnClusterAtPath(t, cluster, client, "ns1/pki")


does this make sense in OSS?

OSS doesn't have namespaces, but it does have nested paths. So this does work and mounts it at ns1/pki, it just isn't a true Enterprise namespace.

builtin/logical/pki/acme_billing_test.go

kitography · 2023-05-15T10:52:35Z

builtin/logical/pki/path_acme_test.go

+
+	namespace := ""
+	mountName := mount
+	if mount != "pki" {


why do we not set up mount "pki" here? Like, surely setupAcmeBackendOnClusterAtPath(t, cluster, client, "pki") should mount pki at path pki?

pki is already mounted by another helper that calls this one.

(setupAcmeBackend in particular).

vault/acme_billing_system_view.go

cipherboy · 2023-05-15T15:00:47Z

@miagilepner @stevendpclark @kitography Ok, updated with review feedback. Let me know what y'all think :-)

builtin/logical/pki/acme_billing_test.go

cipherboy · 2023-05-16T13:32:12Z

@miagilepner Updated! :-)

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

This begins to add custom types of events; presently these are counted as non-entity tokens, but prefixed with a custom ClientID prefix. In the future, this will be the basis for counting these events separately (into separate buckets and separate storage segments). Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

mpalmi · 2023-05-17T14:54:59Z

vault/acme_billing_system_view.go

+	clientID := base64.RawURLEncoding.EncodeToString(identifiersHash[:])
+
+	// Log so users can correlate ACME requests to client count tokens.
+	activityType := "acme"


Would you mind making this a const? It will be useful for the client_type changes that I'm working on. No worries if not. I can make that change as well.

Sure, I can make it a const, but I was a little unclear where I wanted it to live. I think ideally, if we were to hoist it to a proper interface it should probably live in sdk/logical/ and each consumer of the interface would then define their own constant there that everyone could rely on?

But since this is an internal-ish interface only for use by ACME right now, I was thinking that leaving it here (as an internal implementation detail) would be good for now...

Thoughts?

Works for me!

vault/activity_log.go

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Without an additional parameter, SystemView could be of a different internal implementation type that cannot be directly casted to in OSS. Use a separate parameter for the managed key system view to use instead. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Co-authored-by: miagilepner <mia.epner@hashicorp.com> Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Co-authored-by: Mike Palmiotto <mike.palmiotto@hashicorp.com> Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

cipherboy · 2023-05-17T16:05:40Z

Thanks all for the reviews! Setting to auto-merge :-)

cipherboy added enhancement core Issues and Pull-Requests specific to Vault Core secret/pki pr/no-changelog labels May 4, 2023

cipherboy added this to the 1.14 milestone May 4, 2023

cipherboy requested review from kitography, stevendpclark, ncabatoff, miagilepner, mpalmi, hc-github-team-secure-vault-core and a team May 4, 2023 19:39

cipherboy commented May 4, 2023

View reviewed changes

stevendpclark reviewed May 5, 2023

View reviewed changes

builtin/logical/pki/acme_billing_test.go Outdated Show resolved Hide resolved

miagilepner reviewed May 15, 2023

View reviewed changes

kitography reviewed May 15, 2023

View reviewed changes

cipherboy force-pushed the cipherboy-start-billing-acme branch from 917ee5b to ee2fa79 Compare May 15, 2023 14:55

cipherboy force-pushed the cipherboy-start-billing-acme branch from 19092a0 to 0c17226 Compare May 15, 2023 15:51

miagilepner reviewed May 16, 2023

View reviewed changes

builtin/logical/pki/acme_billing_test.go Outdated Show resolved Hide resolved

builtin/logical/pki/acme_billing_test.go Outdated Show resolved Hide resolved

cipherboy force-pushed the cipherboy-start-billing-acme branch from 0c17226 to 8882a3e Compare May 16, 2023 13:31

cipherboy added 4 commits May 17, 2023 10:45

Add stub ACME billing interfaces

bd2423c

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Add initial implementation of client count

ab07003

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Correctly attribute to mount, namespace

d7381ec

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

cipherboy force-pushed the cipherboy-start-billing-acme branch from 8882a3e to f232ed6 Compare May 17, 2023 14:45

mpalmi reviewed May 17, 2023

View reviewed changes

vault/activity_log.go Outdated Show resolved Hide resolved

mpalmi approved these changes May 17, 2023

View reviewed changes

cipherboy and others added 8 commits May 17, 2023 11:39

Refactor creation of ACME mounts

f662dad

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Add test case for billing

1f29b2f

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Refactor creation of mounts for enterprise

72dac9b

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Validate mounts in ACME billing tests

f8d4bf7

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Use a hopefully unique separator for encoded identifiers

6db4efe

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Use mount accesor, not path

3b64ef7

Co-authored-by: miagilepner <mia.epner@hashicorp.com> Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Rename AddEventToFragment->AddActivityToFragment

0c402e5

Co-authored-by: Mike Palmiotto <mike.palmiotto@hashicorp.com> Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

cipherboy force-pushed the cipherboy-start-billing-acme branch from c75dc21 to 0c402e5 Compare May 17, 2023 15:39

cipherboy enabled auto-merge (squash) May 17, 2023 16:05

cipherboy merged commit d234111 into main May 17, 2023

cipherboy deleted the cipherboy-start-billing-acme branch May 17, 2023 16:12

This was referenced Jun 13, 2023

backports for ActivityLog and Reporting 1.13.x #21140

Merged

backports for Activitylog and Reporting 1.12.x #21133

Merged

backports for ActivityLog and Reporting 1.11.x #21210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start counting ACME certificate issuance as client activity #20520

Start counting ACME certificate issuance as client activity #20520

cipherboy commented May 4, 2023

cipherboy May 4, 2023

miagilepner May 11, 2023

cipherboy May 15, 2023

stevendpclark left a comment

miagilepner May 11, 2023

cipherboy May 15, 2023

miagilepner May 11, 2023

kitography left a comment

kitography May 15, 2023

cipherboy May 15, 2023

kitography May 15, 2023

cipherboy May 15, 2023

cipherboy May 15, 2023

cipherboy commented May 15, 2023

cipherboy commented May 16, 2023

mpalmi May 17, 2023

cipherboy May 17, 2023 •

edited

Loading

mpalmi May 17, 2023

cipherboy commented May 17, 2023

Start counting ACME certificate issuance as client activity #20520

Start counting ACME certificate issuance as client activity #20520

Conversation

cipherboy commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevendpclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitography left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy commented May 15, 2023

cipherboy commented May 16, 2023

Choose a reason for hiding this comment

cipherboy May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy commented May 17, 2023

cipherboy May 17, 2023 •

edited

Loading