Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gcp): enable organization validation #2133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions authority/provisioner/gcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"go.step.sm/crypto/sshutil"
"go.step.sm/crypto/x509util"

"github.com/smallstep/certificates/authority/provisioner/gcp"
"github.com/smallstep/certificates/errs"
"github.com/smallstep/certificates/webhook"
)
Expand Down Expand Up @@ -71,6 +72,12 @@ func newGCPConfig() *gcpConfig {
}
}

// projectValidator is an interface to enable testing without using
// gcp.OrganizationProjectValidator.
type projectValidator interface {
ValidateProject(ctx context.Context, projectID string) error
}

// GCP is the provisioner that supports identity tokens created by the Google
// Cloud Platform metadata API.
//
Expand All @@ -93,6 +100,7 @@ type GCP struct {
Name string `json:"name"`
ServiceAccounts []string `json:"serviceAccounts"`
ProjectIDs []string `json:"projectIDs"`
OrganizationID string `json:"organizationID"`
DisableCustomSANs bool `json:"disableCustomSANs"`
DisableTrustOnFirstUse bool `json:"disableTrustOnFirstUse"`
DisableSSHCAUser *bool `json:"disableSSHCAUser,omitempty"`
Expand All @@ -103,6 +111,7 @@ type GCP struct {
config *gcpConfig
keyStore *keyStore
ctl *Controller
projectValidator projectValidator
}

// GetID returns the provisioner unique identifier. The name should uniquely
Expand Down Expand Up @@ -233,14 +242,22 @@ func (p *GCP) Init(config Config) (err error) {
}

config.Audiences = config.Audiences.WithFragment(p.GetIDForToken())
p.ctl, err = NewController(p, p.Claims, config, p.Options)

if p.ctl, err = NewController(p, p.Claims, config, p.Options); err != nil {
return
}

if p.projectValidator, err = gcp.NewOrganizationValidator(context.Background(), p.ProjectIDs, p.OrganizationID); err != nil {
return
}

return
}

// AuthorizeSign validates the given token and returns the sign options that
// will be used on certificate creation.
func (p *GCP) AuthorizeSign(ctx context.Context, token string) ([]SignOption, error) {
claims, err := p.authorizeToken(token)
claims, err := p.authorizeToken(ctx, token)
if err != nil {
return nil, errs.Wrap(http.StatusInternalServerError, err, "gcp.AuthorizeSign")
}
Expand Down Expand Up @@ -315,7 +332,7 @@ func (p *GCP) assertConfig() {
// authorizeToken performs common jwt authorization actions and returns the
// claims for case specific downstream parsing.
// e.g. a Sign request will auth/validate different fields than a Revoke request.
func (p *GCP) authorizeToken(token string) (*gcpPayload, error) {
func (p *GCP) authorizeToken(ctx context.Context, token string) (*gcpPayload, error) {
jwt, err := jose.ParseSigned(token)
if err != nil {
return nil, errs.Wrap(http.StatusUnauthorized, err, "gcp.authorizeToken; error parsing gcp token")
Expand Down Expand Up @@ -368,17 +385,8 @@ func (p *GCP) authorizeToken(token string) (*gcpPayload, error) {
}

// validate projects
if len(p.ProjectIDs) > 0 {
var found bool
for _, pi := range p.ProjectIDs {
if pi == claims.Google.ComputeEngine.ProjectID {
found = true
break
}
}
if !found {
return nil, errs.Unauthorized("gcp.authorizeToken; invalid gcp token - invalid project id")
}
if err := p.projectValidator.ValidateProject(ctx, claims.Google.ComputeEngine.ProjectID); err != nil {
return nil, err
}

// validate instance age
Expand Down Expand Up @@ -414,7 +422,7 @@ func (p *GCP) AuthorizeSSHSign(ctx context.Context, token string) ([]SignOption,
return nil, err
}

claims, err := p.authorizeToken(token)
claims, err := p.authorizeToken(ctx, token)
if err != nil {
return nil, errs.Wrap(http.StatusInternalServerError, err, "gcp.AuthorizeSSHSign")
}
Expand Down
78 changes: 78 additions & 0 deletions authority/provisioner/gcp/projectvalidator.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package gcp

import (
"context"
"net/http"

"github.com/smallstep/certificates/errs"

"google.golang.org/api/cloudresourcemanager/v1"
)

type ProjectValidator struct {
ProjectIDs []string
}

func (p *ProjectValidator) ValidateProject(_ context.Context, projectID string) error {
if len(p.ProjectIDs) == 0 {
return nil
}

for _, pi := range p.ProjectIDs {
if pi == projectID {
return nil
}
}

return errs.Unauthorized("gcp.authorizeToken; invalid gcp token - invalid project id")
}

type OrganizationValidator struct {
*ProjectValidator

OrganizationID string
projectsService *cloudresourcemanager.ProjectsService
}

func NewOrganizationValidator(ctx context.Context, projectIDs []string, organizationID string) (*OrganizationValidator, error) {
crm, err := cloudresourcemanager.NewService(ctx)

if err != nil {
return nil, err
}

return &OrganizationValidator{
&ProjectValidator{projectIDs},
organizationID,
crm.Projects,
}, nil
}

func (p *OrganizationValidator) ValidateProject(ctx context.Context, projectID string) error {
err := p.ProjectValidator.ValidateProject(ctx, projectID)

if p.OrganizationID == "" {
return err
}
Comment on lines +52 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the project validation only enforced when the organizationID is not set?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question - because I structured these as complementary, not mutually exclusive. I'm open to changing that, but for now it's implemented as "if you're not in the project list but an organization ID is set and you're in the organization, you're okay". I believe this would allow an incremental adoption for users already using the project ID list feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes more sense to return an error even if the organization id is set.

I don't know if they are mutually exclusive, but if the project id is in the token and it doesn't match the ones in my configuration, I would expect an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make that change?

Copy link
Author

@ericnorris ericnorris Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make that change, but let me explain - if you set the projectIDs and organizationID, would you not be confused that it was rejected when a token was in fact inside the organization?

That's what I meant by them being mutually exclusive: if it worked the way you described, you could set either projectIDs or organizationID, but not both. I was thinking about how if someone had set projectIDs today, and wanted to try out the organizationID setting, rather than completely disabling projectIDs and enabling organizationID, they could enable organizationID, try it out with a few projects, and then remove projectIDs.

Or maybe they want to allow their organization and one or a few projects from a separate organization? This would also require them to be complementary.

Again though, this is not something I personally feel strongly about, but my hunch is that this behavior is more user-friendly.


ancestry, err := p.projectsService.
GetAncestry(projectID, &cloudresourcemanager.GetAncestryRequest{}).
Context(ctx).
Do()

if err != nil {
return errs.Wrap(http.StatusInternalServerError, err, "gcp.authorizeToken")
}

if len(ancestry.Ancestor) < 1 {
return errs.InternalServer("gcp.authorizeToken; getAncestry response malformed")
}

progenitor := ancestry.Ancestor[len(ancestry.Ancestor)-1]

if progenitor.ResourceId.Type != "organization" || progenitor.ResourceId.Id != p.OrganizationID {
return errs.Unauthorized("gcp.authorizeToken; invalid gcp token - project does not belong to organization")
}

return nil
}
18 changes: 18 additions & 0 deletions authority/provisioner/gcp/projectvalidator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package gcp

import (
"context"
"testing"
)

func TestProjectValidator(t *testing.T) {
validator := &ProjectValidator{ProjectIDs: []string{"allowed-1", "allowed-2"}}

if err := validator.ValidateProject(context.Background(), "not-allowed"); err == nil {
t.Errorf("ProjectValidator.ValidateProject() = nil, want err")
}

if err := validator.ValidateProject(context.Background(), "allowed-2"); err != nil {
t.Errorf("ProjectValidator.ValidateProject() = %v, want nil", err)
}
}
7 changes: 4 additions & 3 deletions authority/provisioner/gcp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

"github.com/smallstep/assert"
"github.com/smallstep/certificates/api/render"
"github.com/smallstep/certificates/authority/provisioner/gcp"
)

func TestGCP_Getters(t *testing.T) {
Expand Down Expand Up @@ -292,7 +293,7 @@ func TestGCP_authorizeToken(t *testing.T) {
"fail/invalid-projectID": func(t *testing.T) test {
p, err := generateGCP()
assert.FatalError(t, err)
p.ProjectIDs = []string{"foo", "bar"}
p.projectValidator = &gcp.ProjectValidator{ProjectIDs: []string{"foo", "bar"}}
tok, err := generateGCPToken(p.ServiceAccounts[0],
"https://accounts.google.com", p.GetID(),
"instance-id", "instance-name", "project-id", "zone",
Expand Down Expand Up @@ -398,7 +399,7 @@ func TestGCP_authorizeToken(t *testing.T) {
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
tc := tt(t)
if claims, err := tc.p.authorizeToken(tc.token); err != nil {
if claims, err := tc.p.authorizeToken(context.Background(), tc.token); err != nil {
if assert.NotNil(t, tc.err) {
var sc render.StatusCodedError
assert.Fatal(t, errors.As(err, &sc), "error does not implement StatusCodedError interface")
Expand Down Expand Up @@ -430,7 +431,7 @@ func TestGCP_AuthorizeSign(t *testing.T) {

p3, err := generateGCP()
assert.FatalError(t, err)
p3.ProjectIDs = []string{"other-project-id"}
p3.projectValidator = &gcp.ProjectValidator{ProjectIDs: []string{"other-project-id"}}
p3.ServiceAccounts = []string{"[email protected]"}
p3.InstanceAge = Duration{1 * time.Minute}

Expand Down
3 changes: 3 additions & 0 deletions authority/provisioner/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"go.step.sm/crypto/pemutil"
"go.step.sm/crypto/randutil"
"golang.org/x/crypto/ssh"

"github.com/smallstep/certificates/authority/provisioner/gcp"
)

var (
Expand Down Expand Up @@ -374,6 +376,7 @@ func generateGCP() (*GCP, error) {
keySet: jose.JSONWebKeySet{Keys: []jose.JSONWebKey{*jwk}},
expiry: time.Now().Add(24 * time.Hour),
},
projectValidator: &gcp.ProjectValidator{},
}
p.ctl, err = NewController(p, p.Claims, Config{
Audiences: testAudiences.WithFragment("gcp/" + name),
Expand Down
Loading