Readonly access keys #870

mxyng · 2022-02-02T00:17:57Z

Revert parts of #860 which moved access key creation to inside infra. This change made it impossible for users to back up and restore keys or migrate tokens between installs.

root|engine access key, restore to original creation process
1. if keys already exist in their respective kubernetes secrets, reuse those values
2. if keys are provided through helm values, template those values into kubernetes secrets
3. otherwise generate a value through helm
encryption key was originally written to a kubernetes secret. the default behaviour now is to write it to a file. default path is alongside the db file.
- this tries to strike a balance between portability since the database is already stored on disk, and usability since it will be transparent to the user
- in production, users should use an external database. as a result, this encryption key should ideally also be external. this can take the form of a kubernetes secret created out of band and injected into the pod through Upgrade Helm chart #848's envFrom or volumes and volumeMounts options or through integrations like vault

Wrote appropriate unit tests
Considered security implications of the change
Updated associated docs where necessary
Updated associated configuration where necessary
Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
Nothing sensitive logged

Resolves #

mxyng · 2022-02-02T03:11:17Z

internal/registry/config.go

 			Permissions: strings.Join(v.Permissions, " "),
-			ExpiresAt:   time.Now().Add(time.Hour * 876000),
+			ExpiresAt:   time.Now().Add(math.MaxInt64),


root tokens shouldn't expire since it's a last resort. it'll be devastating if the backup access method fails because it expired. leaving ExpiresAt empty will cause it to expire very quickly so setting this to the max allowed value for time.Add()

mxyng · 2022-02-02T03:13:07Z

internal/registry/config.go

@@ -487,42 +488,48 @@ func (r *Registry) importAPITokens() error {
 	}

 	for k, v := range keys {
+		if v.Secret == "" {
+			logging.S.Debugf("%s: unset secret", k)


if either reference or value is empty, the token won't be created

mxyng · 2022-02-02T03:14:22Z

internal/registry/models/api_token.go

@@ -17,6 +20,7 @@ type APIToken struct {
 	// TODO: remove me with machine identities
 	Permissions string

+	Key            string


this is necessary otherwise deleting and recreating API tokens will fail since the ID is the primary key

jmorganca · 2022-02-02T03:44:58Z

internal/registry/data/group_test.go

-	var (
-		everyone = models.Group{Name: "Everyone", ProviderID: providerID}
-	)
+	everyone := models.Group{Name: "Everyone", ProviderID: providerID}


Thanks for cleaning this up

BruceMacD

Looks good, my only concern is around the case of changing a specified token secret.

From the rename PR (to simplify find and replace in your change):

rootAPIToken -> systemAccessKey
root-api-token -> system-access-key
engineAPIToken -> engineAccessKey
api-token -> access-key

internal/registry/config.go

helm/charts/infra/templates/secret.yaml

internal/registry/config.go

ssoroka · 2022-02-02T18:26:07Z

internal/registry/config.go

-			if err != nil {
-				return fmt.Errorf("import access keys: %w", err)
-			}
+		parts := strings.Split(raw, ".")


should this be ":" ?

this is the value of the key which is <id>.<secret>

internal/registry/config.go

internal/registry/models/access_key.go

- check error instead of check non-error

mxyng force-pushed the readonly-access-keys branch 3 times, most recently from c22a79e to 4f9773a Compare February 2, 2022 03:08

mxyng commented Feb 2, 2022

View reviewed changes

mxyng requested a review from ssoroka February 2, 2022 03:14

jmorganca reviewed Feb 2, 2022

View reviewed changes

jmorganca approved these changes Feb 2, 2022

View reviewed changes

BruceMacD reviewed Feb 2, 2022

View reviewed changes

internal/registry/config.go Outdated Show resolved Hide resolved

mxyng force-pushed the readonly-access-keys branch 2 times, most recently from 6de0511 to ccc9dd6 Compare February 2, 2022 17:43

mxyng added 2 commits February 2, 2022 09:50

revert access key changes

d17f325

store database encryption key with database file

a0130d2

mxyng force-pushed the readonly-access-keys branch from ccc9dd6 to 73cd452 Compare February 2, 2022 17:50

also check secret checksum

b10933e

mxyng force-pushed the readonly-access-keys branch from 73cd452 to b10933e Compare February 2, 2022 18:06

ssoroka reviewed Feb 2, 2022

View reviewed changes

mxyng added 2 commits February 2, 2022 13:39

AccessKey.Key unqiue index

1225cb7

aggregate errors

aa90494

- check error instead of check non-error

mxyng force-pushed the readonly-access-keys branch from 1773559 to aa90494 Compare February 2, 2022 21:39

mxyng merged commit b757256 into main Feb 2, 2022

mxyng deleted the readonly-access-keys branch February 2, 2022 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Readonly access keys #870

Readonly access keys #870

mxyng commented Feb 2, 2022 •

edited

Loading

mxyng Feb 2, 2022 •

edited

Loading

mxyng Feb 2, 2022

mxyng Feb 2, 2022

jmorganca Feb 2, 2022

BruceMacD left a comment

ssoroka Feb 2, 2022

mxyng Feb 2, 2022

Readonly access keys #870

Readonly access keys #870

Conversation

mxyng commented Feb 2, 2022 • edited Loading

mxyng Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

mxyng Feb 2, 2022

Choose a reason for hiding this comment

mxyng Feb 2, 2022

Choose a reason for hiding this comment

jmorganca Feb 2, 2022

Choose a reason for hiding this comment

BruceMacD left a comment

Choose a reason for hiding this comment

ssoroka Feb 2, 2022

Choose a reason for hiding this comment

mxyng Feb 2, 2022

Choose a reason for hiding this comment

mxyng commented Feb 2, 2022 •

edited

Loading

mxyng Feb 2, 2022 •

edited

Loading