-
Notifications
You must be signed in to change notification settings - Fork 58
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
spread/google: add support for service accounts #187
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,6 @@ import ( | |
"golang.org/x/net/context" | ||
"golang.org/x/oauth2" | ||
"golang.org/x/oauth2/google" | ||
"golang.org/x/oauth2/jwt" | ||
|
||
"github.com/niemeyer/pretty" | ||
"regexp" | ||
|
@@ -44,6 +43,8 @@ type googleProvider struct { | |
|
||
client *http.Client | ||
|
||
serviceAccount string | ||
|
||
mu sync.Mutex | ||
|
||
keyChecked bool | ||
|
@@ -459,6 +460,20 @@ func (p *googleProvider) createMachine(ctx context.Context, system *System) (*go | |
}, | ||
} | ||
|
||
if system.AttachServiceAccount { | ||
if p.serviceAccount == "" { | ||
return nil, &FatalError{fmt.Errorf("no service account to attach")} | ||
} | ||
// XXX the service account could be set from google key | ||
// credentials, but the account used in the context of the | ||
// request may not have the permissions to attach a service | ||
// account to the instance | ||
params["serviceAccounts"] = []googleParams{{ | ||
"email": p.serviceAccount, | ||
"scopes": []string{"https://www.googleapis.com/auth/cloud-platform"}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do the scopes also need to be parameterized (fewer/more/different scopes), or is that the only sensible value to be used when using Spread? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIU it should match the same default scope we use when setting up the client https://github.com/snapcore/spread/blob/ded9133cdbceaf01f8a1c9decf6ff9ea56e194d6/spread/google.go#L776 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This scope feels too powerful to give to an instance running, Do we know the minimum scopes we need for ubuntu pro? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the exact same scope we use when creating GCP client in https://github.com/snapcore/spread/blob/master/spread/google.go#L776 it's also the scope of the token, but the actual permissions. What you can do is not driven by the token scope but by the IAM role assigned to your account. In fact, your application default account, or even the github account we use in default in snapd spred are unable to attach the instances because they lack the necessary permissions in their assigned roles roles. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But the GCP client is spread, right? and the key owner is the one invoking spread (consciously) so it is expected and not a security issue here.
Yes, but since the user can run spread that means their attached svc account can do CRUD operations on GCP instances. My worry, is giving this power to an Ubuntu instance with lots of installed software (large attack surface), all of which would be able to do CRUD operations on the company's internal GCP account. The attack surface grows from just spread's code base, to Ubuntu + a bunch of installed software. Again, I might be missing a puzzle piece here or some context that could make this concern a non issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You cannot attach a service account unless the role assigned to your account is allowed to do so. The application default account all of use does not have that. The service account used by spread runners does not have that either. In fact you even won't be able to start an instance with the service account attached, all you get back is:
You are not giving power to anyone, unless you generate a key for them in GCP admin console and give them the key they can use to pass to spread. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was getting my concerns from https://cloud.google.com/iam/docs/attach-service-accounts:
but since you tested it on launched machine, then I am confusing this with something else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's the whole point of attaching a service account to the machine. This way we as long as the account we use entitled to use Ubuntu Pro, the ubuntu-advantage-client will be able to automatically enable relevant support at boot time. There is no need to share or carry yet another token. |
||
}} | ||
} | ||
|
||
if system.SecureBoot { | ||
params["shieldedInstanceConfig"] = googleParams{ | ||
"enableSecureBoot": true, | ||
|
@@ -755,6 +770,28 @@ func (p *googleProvider) waitOperation(ctx context.Context, s *googleServer, ver | |
panic("unreachable") | ||
} | ||
|
||
func serviceAccountFromKey(raw []byte) (string, error) { | ||
const serviceAccountKey = "service_account" | ||
|
||
// taken from golang.org/x/oauth/google | ||
var credentials struct { | ||
Type string `json:"type"` | ||
ClientEmail string `json:"client_email"` | ||
} | ||
|
||
if err := json.Unmarshal(raw, &credentials); err != nil { | ||
return "", err | ||
} | ||
|
||
if credentials.Type != serviceAccountKey { | ||
// email is only relevant if dealing with service_account | ||
// credentials type | ||
return "", nil | ||
} | ||
|
||
return credentials.ClientEmail, nil | ||
} | ||
|
||
func (p *googleProvider) checkKey() error { | ||
p.mu.Lock() | ||
defer p.mu.Unlock() | ||
|
@@ -771,15 +808,29 @@ func (p *googleProvider) checkKey() error { | |
|
||
if err == nil && p.client == nil { | ||
ctx := context.Background() | ||
if strings.HasPrefix(p.backend.Key, "{") { | ||
var cfg *jwt.Config | ||
cfg, err = google.JWTConfigFromJSON([]byte(p.backend.Key), googleScope) | ||
var creds *google.Credentials | ||
if p.backend.Key != "" { | ||
var raw []byte | ||
if strings.HasPrefix(p.backend.Key, "{") { | ||
// already a raw JSON blob | ||
raw = []byte(p.backend.Key) | ||
} else { | ||
raw, err = ioutil.ReadFile(p.backend.Key) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is just os.ReadFile now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. spread is go 1.13 😞 |
||
} | ||
|
||
if err == nil { | ||
p.client = oauth2.NewClient(ctx, cfg.TokenSource(ctx)) | ||
creds, err = google.CredentialsFromJSON(ctx, raw, googleScope) | ||
} | ||
|
||
// identify service account if possible | ||
p.serviceAccount, err = serviceAccountFromKey(raw) | ||
} else { | ||
os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", p.backend.Key) | ||
p.client, err = google.DefaultClient(context.Background(), googleScope) | ||
// none provided, let the google library find whatever | ||
// is appropriate | ||
creds, err = google.FindDefaultCredentials(ctx, googleScope) | ||
} | ||
if err == nil { | ||
p.client = oauth2.NewClient(ctx, creds.TokenSource) | ||
} | ||
} | ||
if err == nil { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make this more realistic or more clarly a placeholder text.