Skip to content

rbac: dbauthz fails closed when fetching related resources #262

Open
@johnstcn

Description

@johnstcn
Member

Problem:

Given:

  • You have resource A related to resource B
  • You authorize actions on resource A by checking relevant permissions on resource B
  • Resource A exists, but for some reason references to a nonexistant resource B

When:

  • You ask if you can do X on A

Then:

  • You get sql.ErrNoRows relating to the fetch of resource B, which is completely unintuitive

This was encountered when writing a test case for provisioner jobs using dbgen.ProvisionerJob, but I could imagine a situation where this could arise in the "real world".

Example to reproduce:

	var defOrgID uuid.UUID
	if orig.OrganizationID == uuid.Nil {
		defOrg, _ := db.GetDefaultOrganization(genCtx)
		defOrgID = defOrg.ID
	}

	jobID := takeFirst(orig.ID, uuid.New())
	// Always set some tags to prevent Acquire from grabbing jobs it should not.
	tags := takeFirstMap(orig.Tags, database.StringMap{"user": "", "scope": "organization"})
	if orig.Tags == nil && !orig.StartedAt.Time.IsZero() {
		// Make sure when we acquire the job, we only get this one.
		tags[jobID.String()] = "true"
	}

	job, err := db.InsertProvisionerJob(genCtx, database.InsertProvisionerJobParams{
		ID:             jobID,
		CreatedAt:      takeFirst(orig.CreatedAt, dbtime.Now()),
		UpdatedAt:      takeFirst(orig.UpdatedAt, dbtime.Now()),
		OrganizationID: takeFirst(orig.OrganizationID, defOrgID, uuid.New()),
		InitiatorID:    takeFirst(orig.InitiatorID, uuid.New()),
		Provisioner:    takeFirst(orig.Provisioner, database.ProvisionerTypeEcho),
		StorageMethod:  takeFirst(orig.StorageMethod, database.ProvisionerStorageMethodFile),
		FileID:         takeFirst(orig.FileID, uuid.New()),
		Type:           takeFirst(orig.Type, database.ProvisionerJobTypeWorkspaceBuild),
		Input:          takeFirstSlice(orig.Input, []byte("{}")),
		Tags:           tags,
		TraceMetadata:  pqtype.NullRawMessage{},
	})
	_ = job
	require.NoError(t, err, "insert job")
	job, err = db.GetProvisionerJobByID(genCtx, jobID)
	require.NoError(t, err, "get job: %s", jobID.String())

Suggested solution:

Fail open if the related resource is not present instead.

cc @mafredri

Activity

johnstcn

johnstcn commented on Dec 20, 2024

@johnstcn
MemberAuthor

cc @Emyrk wondering what your thoughts are here?

mafredri

mafredri commented on Dec 20, 2024

@mafredri
Member

Here's the test code (utilizing dbgen) that triggered the issue (sql: no rows):

		d1 := dbgen.ProvisionerDaemon(t, coderdAPI.Database, database.ProvisionerDaemon{
			Name:  "provisioner-0",
			KeyID: uuid.MustParse(codersdk.ProvisionerKeyIDBuiltIn),
		})
		dbgen.ProvisionerJob(t, coderdAPI.Database, coderdAPI.Pubsub, database.ProvisionerJob{
			WorkerID:  uuid.NullUUID{UUID: d1.ID, Valid: true},
			StartedAt: sql.NullTime{Time: coderdAPI.Clock.Now(), Valid: true},
			Tags:      database.StringMap{"user": "", "scope": "organization"},
		})
Emyrk

Emyrk commented on Dec 20, 2024

@Emyrk
Member

@johnstcn I think in practice this should be enforced with foreign key dependencies, if we do not already. In production this should be impossible.

dbauthz_tests uses the in mem db, and @hugodutka made this to support it using the actual sql: https://github.com/coder/coder/blob/2f188e8c94645470b9030312e379016adc9ef4e9/coderd/database/queries/testadmin.sql

This feels like an inevitable if there is not enough context in the resource A. I think the 404 behavior is correct, and the database at that point would be "corrupted". The resource A is effectively orphaned.

A note:

If this can happen, for w/e reason, and is expected, we can do what we do for files.
https://github.com/coder/coder/blob/e931831d96fb216f816b4532daa346b1842d8924/coderd/database/dbauthz/dbauthz.go#L1606-L1620

There is 2 access methods to the file. The file directly, or via a template.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @mafredri@johnstcn@Emyrk

        Issue actions

          rbac: dbauthz fails closed when fetching related resources · Issue #262 · coder/internal