KMS cannot Get, Set, or Test IAM

[sethvargo]

Client

KMS

Describe Your Environment

MacBook Pro (15-inch, 2017), 10.14.2 (18C54)
2.8 GHz Intel Core i7
16 GB 2133 MHz LPDDR3
Radeon Pro 555 2048 MB
Intel HD Graphics 630 1536 MB

But also reproducible everywhere

Expected Behavior

KMS clients should be able to Get, Set, and Test IAM policies.

Actual Behavior

Performing any Get, Set, or Test operations using the iam.Handle returns the following error:

rpc error: code = NotFound desc = The request concerns location 'us-central1' but was sent to location 'global'. Either Cloud KMS is not available in 'us-central1' or the request was misrouted. Make sure the 'x-goog-request-params' metadata is set correctly; see https://cloud.google.com/kms/docs/grpc for more information.

Where "us-central1" corresponds to the region in which my KMS keys exist. I conferred with the KMS team, and it appears that the x-goog-request-params is not set on IAM calls, which appears to be the root cause.

Reproduction

Run this sample, changing the keyRing constant to the full resource URL of a keyring in a project in any location other than "global".

package main

import (
	"context"
	"log"

	cloudkms "cloud.google.com/go/kms/apiv1"
	kmspb "google.golang.org/genproto/googleapis/cloud/kms/v1"
)

const keyRing = "projects/my-project/..." // Fill with existing keyring

func main() {
	ctx := context.Background()
	client, err := cloudkms.NewKeyManagementClient(ctx)
	if err != nil {
		panic(err)
	}

	resp, err := client.KeyRingIAM(&kmspb.KeyRing{
		Name: keyRing,
	}).Policy(context.Background())
	if err != nil {
		panic(err)
	}

	log.Printf("#%v", resp)
}

Diagnoses

The KeyRingIAM func creates a handle wrapper around the KMS client's Connection object which drops straight into the iam package. There doesn't appear to be a way to pass additional metadata pairs to the IAM client, so when gax invokes the policy requests, they fail.

I confirmed the API works as-expected with the headers on the request by adding the following method to the KMS client itself so I can access the request fields:

func (c *KeyManagementClient) GetKeyRingIAM(ctx context.Context, req *kmspb.GetKeyRingRequest, opts ...gax.CallOption) (*iam.Policy, error) {
	md := metadata.Pairs("x-goog-request-params", fmt.Sprintf("%s=%v", "name", req.GetName()))
	ctx = insertMetadata(ctx, c.xGoogMetadata, md)

	// Works with or without the additional options here, kept for parity with other KMS client funcs
	opts = append(c.CallOptions.GetKeyRing[0:len(c.CallOptions.GetKeyRing):len(c.CallOptions.GetKeyRing)], opts...)

	var proto *pb.Policy
	err := gax.Invoke(ctx, func(ctx context.Context, settings gax.CallSettings) error {
		var err error
		// Create the client here so we can pass it the proper context
		iamClient := pb.NewIAMPolicyClient(c.Connection())
		proto, err = iamClient.GetIamPolicy(ctx, &pb.GetIamPolicyRequest{Resource: req.GetName()}, settings.GRPC...)
		return err
	}, opts...)
	if err != nil {
		return nil, err
	}
	return &iam.Policy{InternalProto: proto}, nil
}

Note this function lives in the kms package and injects the x-google-request-params into the context before creating the IAM client. I think this might be the best approach to preserve those headers without significant altercations to the iam package itself, but it means that the client does not given an iam.Handle to the user directly.

Alternatives

• Provide top-level functionality in the IAM package to add headers
• Allow objects to pass a context to the IAM handler and store it on the struct (gross)
• Force uses to manually add metadata fields (even grosser)

[jeanbza]

@jskeet is this something you've observed in C#?

[broady]

Only 5 hits for x-goog-request-params, is this something the API team needs to add to their API definition to get the code generator to generate the right code?

https://github.com/googleapis/google-cloud-go/search?q=x-goog-request-params&unscoped_q=x-goog-request-params

[bdhess]

I'm going to chime in that I don't like the way iam.go is designed anyway, and if we have an opportunity to fix the API, let's.

Specifically, I don't like that there is something to manually maintain, if we choose to expose IAM permissions on additional resource types in the future.

[bdhess]

@broady the IAM methods are handwritten

[broady]

@bdhess ah, right. other APIs do not seem to require x-goog-request-params for IAM methods, is KMS special in this respect? I'm guessing it's fine to add to all requests (i.e., to all APIs), can you confirm?

Specifically, I don't like that there is something to manually maintain, if we choose to expose IAM permissions on additional resource types in the future.

I don't fully understand, can you rephrase? Do you mean that you think IAM functionality should not be hand-written, but auto-generated?

I agree -- I was surprised that the hooks to IAM are manually written.

[bdhess]

There are some other APIs that require the metadata. It's because KMS is regionalized, and GFEs need to forward the request to the correct backend Stubby service.

I'm not saying this shouldn't be handwritten (although the GAPIC generator can generate these for all of the other silver languages). But that I'd prefer a single generic method that just takes a resource path string to multiple strongly-typed methods that take specific kinds of resources.

[jeanbza]

cc @pongad

[jskeet]

I'll have to have a look tomorrow

[jeanbza]

@jskeet May be a false alarm - seems like it's Go specific. Sorry!

[broady]

pinged @bdhess offline to clarify, here's my interpretation on his comments about the IAM methods in the KMS package:

func (c *KeyManagementClient) CryptoKeyIAM(cryptoKey *kmspb.CryptoKey) *iam.Handle
should be
func (c *KeyManagementClient) CryptoKeyIAM(cryptoKeyResourceName string) *iam.Handle

Since only the name is used from the CryptoKey struct, anyway. (See Seth's code above as an example of seemingly unnecessary struct creation.)

Anyway, this might make sense for KMS but not for other APIs, which might not pass around or ask the developer to construct full resource paths as often as KMS does. Worth more investigation and thought (discussion on another thread, probably). Though @bdhess points out that since it's totally busted already, it might be fine to change at the same time as making a bug fix.

---

But it seems like we should just add x-goog-request-params to all IAM requests in the cloud.google.com/go/iam package.

[bdhess]

I'd go further and say

func (c *KeyManagementClient) CryptoKeyIAM(cryptoKey *kmspb.CryptoKey) *iam.Handle
func (c *KeyManagementClient) KeyRingIAM(keyRing *kmspb.KeyRing) *iam.Handle

Should both converge to

func (c *KeyManagementClient) ResourceIAM(resourcePath string) *iam.Handle

Not wedded to the names, but I'd like to get down to just one signature. We're planning to add at least one new resource type this quarter that will have IAM permissions, it would be great to have no changes required to handwritten Go client code in order to support.

[jeanbza]

We foolishly set the gapic to GA in 205084741 before we were aware that the library needed to go through a separate GA process than the service... sigh. :(

We could probably make a breaking change since - if I'm reading this bug right - I doubt this works for anyone?

[bdhess]

This will not work for anyone today unless they went through the trouble to add the correct metadata pairs to the context before making the IAM call. It's not impossible, but seems very unlikely.

[broady]

Is there such a thing as a global KMS keyring, and if so, would this functionality work?

[bdhess]

"global" is just-another-named-region in Cloud KMS, with the property that it is served out of multiple globally redundant data centers. Even for "global", GFEs need to know to address the request to the "global" stubby backend.

[sethvargo]

But it seems like we should just add x-goog-request-params to all IAM requests in the cloud.google.com/go/iam package.

That might be challenging because you don't know what that KV pair is. It varies by resource (sometimes it's "name", "parent", or "resource").

We could probably make a breaking change since - if I'm reading this bug right - I doubt this works for anyone?

It works iff keys are in the "global" region (i.e. a key projects/my-project/locations/global/keyRings/my-keyring).

This will not work for anyone today unless they went through the trouble to add the correct metadata pairs to the context before making the IAM call. It's not impossible, but seems very unlikely.

I don't actually think it's possible today since there's no way to inject metadata into the iam.Handle. The user would have to fully re-construct the grpc call with gax. While that's not impossible, it definitely borders on terrible.

Even for "global", GFEs need to know to address the request to the "global" stubby backend.

In my testing, global does appear to work as-is with this library

[bdhess]

That might be challenging because you don't know what that KV pair is. It varies by resource (sometimes it's "name", "parent", or "resource").

They're well defined for the three IAM methods, though.

It works iff keys are in the "global" region (i.e. a key projects/my-project/locations/global/keyRings/my-keyring).

Confirmed-- there is a default routing in GFEs that I didn't know existed. GFEs will route to our 'global' region in the absence of metadata (and they will also route to the 'global' region if 'global' is provided in the metadata). Withdrawing my request to make a breaking change (though I would like to still consider adding the generic method).

I don't actually think it's possible today since there's no way to inject metadata into the iam.Handle

It doesn't need to be in the iam.Handle; you could add it to the calls made with the handle, e.g. handle.Policy(ctx)

In my testing, global does appear to work as-is with this library

Indeed; I stand corrected.

[sethvargo]

Got it, thanks for the pointer. I was able to modify the context and got this to work:

package main

import (
	"context"
	"fmt"

	cloudkms "cloud.google.com/go/kms/apiv1"
	kmspb "google.golang.org/genproto/googleapis/cloud/kms/v1"
	"google.golang.org/grpc/metadata"
)

const keyRing = "projects/my-project/..." // Fill with existing keyring

func main() {
	ctx := context.Background()
	client, err := cloudkms.NewKeyManagementClient(ctx)
	if err != nil {
		panic(err)
	}

	ctx = metadata.AppendToOutgoingContext(context.Background(),
		"x-goog-request-params", fmt.Sprintf("name=%v", keyRing))
	policy, err := client.KeyRingIAM(&kmspb.KeyRing{Name: keyRing}).Policy(ctx)
	if err != nil {
		panic(err)
	}

	fmt.Printf("Retrieved policy: %#v\n", policy)
}

That's actually not horrible, but it requires some pretty specific knowledge that I'd expect the client library to take care of for me.

[broady]

A few things, maybe @pongad can help...

(1) the IAM gapic isn't generated (though the "admin" and "credentials" packages are) even though it seems to be specified under "language_settings" in the gapic config. Is there some other manual step I don't know about? Is there a reason we don't have this generated? Is that our policy for packages that have hand-written clients? (Makes sense. No gapic for bigtable, datastore, etc. but there is for spanner.)

(2) should we then add the header_request_params definition to iam_gapic.yaml?

(3) should we then have cloud.google.com/go/iam depend on the gapic?

If no to (3) then let's just add it manually to the existing hand-written iam package.

[bdhess]

I'll also add, though it's somewhat tangential to this issue, that Cloud KMS exposes the google.cloud.locations.Locations service, and that is not currently exposed in the Go client. The other GAPIC clients automatically generate GetLocations and ListLocations methods.

[broady]

@bdhess where? I don't see it in the KMS proto service definition, nor do I see it in the Ruby client. Please open another bug if this is not WAI or ping me on chat.

And yes, let's keep this bug specifically to adding x-goog-request-params to the iam package.

[bdhess]

Yeah, I'm wrong, it's not in the GAPIC config at all, so doesn't appear in any of the GAPIC clients. We need to add that, but totally out of scope for this issue. (It's not in the KMS protos though; like IAM, it has its own protos)

[pongad]

I have moved teams, so I'll add @noahdietz who now owns the Go generator.

To answer @broady's question.

1. The manual step to generate a client is essentially to add it to regen-gapic.sh and run the script (or wait for Eno and Jean to run it :P). IIRC we haven't generated a client for IAM mostly because we didn't think we needed one. I don't have a compelling reason to not start generating it.

2. & 3. If we generate the gapic, the answer is yes.

[broady]

I think the fact that anyone that chooses to generate IAM will not be bitten by this bug is a good enough reason to add header_request_params to the gapic yaml (and to use it).

@jadekler wdyt? can do separately, since the fix for this particular bug is likely a 3 LOC delta change.

[sethvargo]

Hey @jadekler @noahdietz - have we decided on the best path forward here? I'm happy to help out, but don't want to spend cycles going in the wrong direction 😄

[noahdietz]

Hi everyone, I'm taking a look at the proposed solution, will get back to y'all soon

[jeanbza]

Hi all, sorry for the delay. Expanding on this plan it looks like we need to:

Ok, tell me what you think about this plan:

• Generate iam/apiv1 gapic
• Make iam/iam.go rely on iam/apiv1 gapic (making sure it sets req.Resource)
• Change kms/iam.go signature to func (c *KeyManagementClient) ResourceIAM(resourcePath string) *iam.Handle
  • Merges CryptoKeyIAM and KeyRingIAM into one
  • Takes a resourcePath and hands it down to iam/iam.go

This works because the gapic client will by default use req.Resource to set x-goog-request-params, unlike the hand-written iam/iam.go methods.

Does this look good to everyone?

edit: btw @broady @pongad @noahdietz I'm not sure how header_request_params fits into any of this. Could one of you describe that?

[sethvargo]

For the third point, I think I'd rather accept an interface than a string. While right now all KMS resources are uniquely identifiable as a string, the future is unknown. Also, if other libs move to this style, they might need a method that defines the unique resource URL.

type IAMResourcePather interface {
  IAMResourceName() string
}

func (c *KeyManagementClient) ResourceIAM(i IAMResourcePather) *IAMHandle

I'm not tied to the names, but the interface might give us more flexibility in the future.

[noahdietz]

The header_request_params will extract the specified fields & their values from the input message, expand them into key=value pairs, and adds them to x-goog-request-params prior to executing the API call. This is done by the GAPIC. A concrete example:

Say we define GetIamPolicy to have the GetIamPolicyRequest.resource field as a header_request_params value. The resulting GAPIC method would generate code like so:

func (c *IamPolicyClient) GetIamPolicy(ctx context.Context, req *iampb.GetIamPolicyRequest, opts ...gax.CallOption) (*iampb.Policy, error) {
	md := metadata.Pairs("x-goog-request-params", fmt.Sprintf("%s=%v", "resource", req.GetResource()))
	ctx = insertMetadata(ctx, c.xGoogMetadata, md)

Per the earlier example, it seems like this is the desired behavior, to have the resource name to be provided in the x-goog-request-params:

ctx = metadata.AppendToOutgoingContext(context.Background(),
		"x-goog-request-params", fmt.Sprintf("name=%v", keyRing))
	policy, err := client.KeyRingIAM(&kmspb.KeyRing{Name: keyRing}).Policy(ctx)

[jeanbza]

@sethvargo I'm 👎 on interfaces: they're harder for users to reason about, much harder for us to document, and harder for us to code around the edge cases. Do we anticipate anything other than string? If so, then I'd rather go back to having per-resource methods with well defined inputs.

@noahdietz Gotcha! Thanks. Then IIUC the amended plan would be:

• Generate iam/apiv1 gapic, making sure to use header_request_params to configure resource=GetIamPolicyRequest.resource
• Make iam/iam.go rely on iam/apiv1 gapic (making sure it sets req.Resource)
• Change kms/iam.go signature to func (c *KeyManagementClient) ResourceIAM(resourcePath string) *iam.Handle
  • Merges CryptoKeyIAM and KeyRingIAM into one
  • Takes a resourcePath and hands it down to iam/iam.go

Does that look right?

[noahdietz]

making sure to use header_request_params to configure resource=GetIamPolicyRequest.resource

Yes, and this entails changes to the google/iam/v1/iam_gapic.yaml.

Also worth noting, Pub/Sub depends on the manual IAM client in a similar way so any changes we make there should be tested with Pub/Sub as well.

[noahdietz]

As I reread this, I'm a little confused on what the x-goog-request-params should be. The examples of "getting it to work" provided by folks here use a name=projects/my-project/.../keyring, where name is a field in the KMS message. But in the IAMPolicy service all of the messages use a resource field.

If the value of KMS name (which looks like a resource path) is paired with resource key in the x-goog-request-params will that still work?

The GAPIC header_request_params config on IAM can only pull field values for those present in the input message i.e. GetIamPolicyRequest

[sethvargo]

name works. I just tested and resource also works. I think we should got for resource unless there's any objections.

[bdhess]

resource is correct for IAM. I don't know how/why name works.

But here's prior art for GAPIC-generated Java code:
https://github.com/googleapis/google-cloud-java/blob/7fc4b3b2973faa23292b4f92f49ef1445d19934d/google-cloud-clients/google-cloud-kms/src/main/java/com/google/cloud/kms/v1/stub/GrpcKeyManagementServiceStub.java#L626

[noahdietz]

Thanks everyone for testing/researching this.

I've parked the changes we've discussed here in a CL.

This will require changes to the relevant GAPIC configurations in IAM (reference PR) which I am figuring out if we can do or if the service owner must do.

[noahdietz]

Hey all so I talked with @lukesneeringer about how this should be done and we weren't sure this was the right approach. Luke, can you weigh in with what you know about the IAM methods. Hopefully I represented things fairly.

CC @wora @jskeet per Luke's request

[lukesneeringer]

As best as I can tell, the issue here seems to be that KMS is different than the rest of Google (except Pub/Sub, a known separate issue) in how it exposes IAM methods. The general way this is done is for the three IAM methods to be delcared within an API's own services. A canonical example is here, but there are many others.

If KMS followed the standard here, we would get the header params from the HTTP binding, as described in {ping me for internal link if you need it}.

I would rather not add these new components to what we generate (and then add a new sort-of hack on top of them) in order to support a non-standard case. My feeling is that either KMS should follow the standard pattern (likely no longer possible since it is in GA) or supporting this should be handled manually.

[jskeet]

Do we yet know the impact of this on other platforms? I haven't looked at every aspect of KMS myself, but I'm aware of a large set of samples being prepared - should I check whether that includes IAM calls?

[jeanbza]

I'll leave this to @bdhess to reply to except to mention that I'm OK making a backwards-incompatible change * since approximately zero users use these methods, and they are roughly broken in their current state by not setting x-goog-request-params.

* If we went this route, I would suggest deprecation + new methods in one release, removal of old method in another release.

[noahdietz]

To Luke's point, here is another service that has (Get|Set|Test)IamPolicy methods in their GAPICs (even with x-goog-request-params resource=X). Was there a reason why the IAM methods were excluded from the KMS service proto (and as a result, the GAPIC)?

If this question & its implied solution is irrelevant b.c of release stuff, please say so 😄 Thanks

[bdhess]

@noahdietz and I caught up offline; I think he will follow up with @lukesneeringer

@jskeet AFAIK this issue is limited to Golang; it seems the Golang GAPIC generator lacks support for reroute_to_grpc_interface, and so is unable to cope with endpoints that expose multiple gRPC services. This works fine in the .NET GAPIC generator and in the resulting Cloud KMS client (sample code here).

[noahdietz]

I spoke with Luke offline and we are good to go here. To restate the plan, we want to:

1. Update the relevant IAM GAPIC configuration to include the header_request_params for resource
2. Generate a new GAPIC for IAMPolicy and refactor the manual IAM client to use this GAPIC
3. Refactor the existing manual KMS IAM resource methods to:
   a. deprecate the existing resource-specific handle wrappers in N releases
   b. add a new func (c *KeyManagementClient) ResourceIAM(resourcePath string) *iam.Handle

Furthermore, I am taking an item to investigate why the reroute_to_grpc_interface configuration isn't being honored in the Go GAPICs here.

[noahdietz]

After digging into the necessary changes to generate an IAMPolicy GAPIC, I'm finding that this is not possible. I spoke too soon. I think we need to make this x-goog-request-params change in the manual layer.

Why? Because the IAMPolicy service does not have a default host. iam.googleapis.com does not serve it. (cloudkms | pubsub | compute | etc.).googleapis.com serve the IAMPolicy service. Thus, generating a GAPIC is not possible, because there is no single default host the published library would target by default. It would be broken by default.

@jadekler @bdhess Can we do this request param addition in one of the manual layers?

Sorry to get hopes up about generating a GAPIC, I am learning as I go and I appreciate your patience.

[noahdietz]

We went with the manual layer approach & added the new ResourceIAM in order to deprecate the resource-specific KMS IAM wrappers.