credentials/alts: Initial version of ALTS code

[cesarghali]

Add support for Google's Application Layer Transport Protocol as a gRPC transport layer protocol.

For more information read the Application Layer Transport Protocol whitepaper:
https://cloud.google.com/security/encryption-in-transit/application-layer-transport-security/

---

This change is 

[therc]

Any documentation?

[jiangtaoli2016]

@therc This code is for silent launch. The code is not ready for public consumption yet. Hopefully soon. Once we are beta launch, we will provide corresponding public user guide.

[therc]

@jiangtaoli2016 I see, thanks. Also, another suggestion for the whitepaper: a section on "Why not Kerberos?". I think I know some of the reasons, but others are bound to ask the same question.

[menghanl]

Only looked at package alts. Will take a look at other internal packages later.

---

Reviewed 2 of 31 files at r1.
Review status: 2 of 31 files reviewed at latest revision, 13 unresolved discussions.

---

credentials/alts/alts.go, line 22 at r1 (raw file):

// encapsulates all the state needed by a client to authenticate with a server
// using ALTS and make various assertions, e.g., about the client's identity,
// role, or whether it is authorized to make a particular call.

Declare this package as experimental?

---

credentials/alts/alts.go, line 53 at r1 (raw file):

var (
	enableUntrustedALTS = flag.Bool("enable_untrusted_alts", false, "Enables ALTS in untrusted mode. Enabling this mode is risky since we cannot ensure that the application is running on GCP with a trusted handshaker service.")

What's the purpose of this flag?

Also, I kind of don't like the idea of adding a flag for it.
How about moving this to constructors NewClientALTS() and NewServerALTS() as a function parameter?

---

credentials/alts/alts.go, line 62 at r1 (raw file):

// AuthInfo exposes security information from the ALTS handshake to the
// application.
type AuthInfo interface {

Should we add to the comment that:

This interface is to be implemented by alts. Users should not need a
brand new implementation of this interface. For the situations like
testing, the new implementation should embed this interface. This allows
alts to add new methods to this interface.

---

credentials/alts/alts.go, line 80 at r1 (raw file):

}

// altsTC is the credentials required for authenticating a connection using Google

Nit: this still says "Google Transport Security".

---

credentials/alts/alts.go, line 129 at r1 (raw file):

	opts := handshaker.DefaultClientHandshakerOptions()
	opts.TargetServiceAccounts = g.accounts
	opts.RPCVersions = &altspb.RpcProtocolVersions{

Make a static variable for versions struct so we don't need to re-create it every time?

---

credentials/alts/alts.go, line 139 at r1 (raw file):

		},
	}
	chs, err := handshaker.NewClientHandshaker(ctx, hsConn, rawConn, opts)

Will chs be properly closed if something goes wrong (e.g. this function returns non-nil error)?

Also see #1854 for possible leaks on streaming RPCs.

---

credentials/alts/alts.go, line 151 at r1 (raw file):

		return nil, nil, errors.New("client-side auth info is not of type alts.AuthInfo")
	}
	match, _ := checkRPCVersions(opts.RPCVersions, altsAuthInfo.PeerRPCVersions())

Does the underlying handshaker need the RPCVersions if the check is done here?

IMO the check should happen before we create the secConn, so we can avoid the overhead in error cases.
It would also be a bit clearer if this check is done in chs.ClientHandshake, so we can save the type assertion.

---

credentials/alts/alts.go, line 173 at r1 (raw file):

	defer cancel()
	opts := handshaker.DefaultServerHandshakerOptions()
	opts.RPCVersions = &altspb.RpcProtocolVersions{

Make a static variable?

---

credentials/alts/alts.go, line 183 at r1 (raw file):

		},
	}
	shs, err := handshaker.NewServerHandshaker(ctx, hsConn, rawConn, opts)

Similar to client side, shs clean up on errors cases.

---

credentials/alts/alts.go, line 195 at r1 (raw file):

		return nil, nil, errors.New("server-side auth info is not of type alts.AuthInfo")
	}
	match, _ := checkRPCVersions(opts.RPCVersions, altsAuthInfo.PeerRPCVersions())

Similar to client side on where the check should happen.

---

credentials/alts/utils.go, line 53 at r1 (raw file):

	// The following two variables will be reassigned in tests.
	runningOS  = runtime.GOOS
	readerFunc = func() (io.Reader, error) {

Nit: rename this function to something more meaningful.

---

credentials/alts/utils.go, line 86 at r1 (raw file):

	manufacturer, err := readManufacturer()
	if err != nil {
		log.Fatal(err)

This fatal will print the error returned by os.Open or cmd.Output.
Do a Fatalf with more information? Or include more information in readerFunc.

---

credentials/alts/utils.go, line 99 at r1 (raw file):

		return name == "Google"
	default:
		panic(platformError(runningOS))

Why is this a panic not a Fatalf?

---

Comments from Reviewable

[cesarghali]

Thanks for the review Menghan. All comments are addressed.

---

Review status: 2 of 31 files reviewed at latest revision, 13 unresolved discussions.

---

credentials/alts/alts.go, line 22 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Declare this package as experimental?

Done.

---

credentials/alts/alts.go, line 53 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

What's the purpose of this flag?

Also, I kind of don't like the idea of adding a flag for it.
How about moving this to constructors NewClientALTS() and NewServerALTS() as a function parameter?

Ack!

---

credentials/alts/alts.go, line 62 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Should we add to the comment that:

This interface is to be implemented by alts. Users should not need a
brand new implementation of this interface. For the situations like
testing, the new implementation should embed this interface. This allows
alts to add new methods to this interface.

Done.

---

credentials/alts/alts.go, line 80 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Nit: this still says "Google Transport Security".

Good catch, thanks!

---

credentials/alts/alts.go, line 129 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Make a static variable for versions struct so we don't need to re-create it every time?

Done.

---

credentials/alts/alts.go, line 139 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Will chs be properly closed if something goes wrong (e.g. this function returns non-nil error)?

Also see #1854 for possible leaks on streaming RPCs.

Done.

---

credentials/alts/alts.go, line 151 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Does the underlying handshaker need the RPCVersions if the check is done here?

IMO the check should happen before we create the secConn, so we can avoid the overhead in error cases.
It would also be a bit clearer if this check is done in chs.ClientHandshake, so we can save the type assertion.

Ack!

---

credentials/alts/alts.go, line 173 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Make a static variable?

Done.

---

credentials/alts/alts.go, line 183 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Similar to client side, shs clean up on errors cases.

Done.

---

credentials/alts/alts.go, line 195 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Similar to client side on where the check should happen.

Ack!

---

credentials/alts/utils.go, line 53 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Nit: rename this function to something more meaningful.

Done.

---

credentials/alts/utils.go, line 86 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

This fatal will print the error returned by os.Open or cmd.Output.
Do a Fatalf with more information? Or include more information in readerFunc.

Done.

---

credentials/alts/utils.go, line 99 at r1 (raw file):

Previously, menghanl (Menghan Li) wrote…

Why is this a panic not a Fatalf?

Done.

---

Comments from Reviewable

[menghanl]

Review status: 0 of 31 files reviewed at latest revision, 4 unresolved discussions.

---

credentials/alts/alts.go, line 87 at r3 (raw file):

	SecurityLevel() altspb.SecurityLevel
	// PeerServiceAccount returns the peer service account.
	PeerServiceAccount() string

Will we want to extend the return value?
"account" sounds like more than just a string to me...
If so, make it a struct instead?

---

credentials/alts/alts.go, line 91 at r3 (raw file):

	LocalServiceAccount() string
	// PeerRPCVersions returns the RPC version supported by the peer.
	PeerRPCVersions() *altspb.RpcProtocolVersions

All functions in this interface will be exposed to users, including this one.

This function is needed by checkRPCVersions().

Will users also need this? If no, this should be moved to a separate (probably also unexported) interface.

---

credentials/alts/alts.go, line 131 at r3 (raw file):

func (g *altsTC) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (_ net.Conn, _ credentials.AuthInfo, err error) {
	// Make sure flags are parsed before accessing enableUntrustedALTS.
	once.Do(func() { flag.Parse() })

Should this be moved to newALTS()?

1. newALTS() also reads *enableUntrustedALTS
2. init should be done in the "package entrance", which should be newALTS() for this package.

---

credentials/alts/utils.go, line 78 at r3 (raw file):

		}
	}
	vmOnGCP = isRunningOnGCP()

Also move this into the sync.Once?
This and flag.Parse() are all part of the initialization process.

Otherwise if users import but don't use alts, they may still get Fatal.

---

Comments from Reviewable

[cesarghali]

Review status: 0 of 31 files reviewed at latest revision, 4 unresolved discussions.

---

credentials/alts/alts.go, line 87 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Will we want to extend the return value?
"account" sounds like more than just a string to me...
If so, make it a struct instead?

ServiceAccount is just a string, usually it's an email address or some other identifier.

---

credentials/alts/alts.go, line 91 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

All functions in this interface will be exposed to users, including this one.

This function is needed by checkRPCVersions().

Will users also need this? If no, this should be moved to a separate (probably also unexported) interface.

All the functions in this interface might be used for the user for logging purposes or to check whether a specific RPC was actually executed as expected. We need all of them to be exported.

---

credentials/alts/alts.go, line 131 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Should this be moved to newALTS()?

1. newALTS() also reads *enableUntrustedALTS
2. init should be done in the "package entrance", which should be newALTS() for this package.

Good call. Done.

---

credentials/alts/utils.go, line 78 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

Also move this into the sync.Once?
This and flag.Parse() are all part of the initialization process.

Otherwise if users import but don't use alts, they may still get Fatal.

I'm not sure I'm following. Do you mean that calling isRunningOnGCP should be called in a once.Do? Should this be in init()?

---

Comments from Reviewable

[cesarghali]

Review status: 0 of 31 files reviewed at latest revision, 4 unresolved discussions.

---

credentials/alts/utils.go, line 78 at r3 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

I'm not sure I'm following. Do you mean that calling isRunningOnGCP should be called in a once.Do? Should this be in init()?

isRunningOnGCP was actually in init() but now I remember why I moved it here, it was one of Doug's suggestions :)

---

Comments from Reviewable

[menghanl]

Reviewed 1 of 1 files at r4.
Review status: 1 of 31 files reviewed at latest revision, 1 unresolved discussion.

---

credentials/alts/utils.go, line 78 at r3 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

isRunningOnGCP was actually in init() but now I remember why I moved it here, it was one of Doug's suggestions :)

I meant moving this to the same sync once for flag.Parse(). Because we don't need to do this if no alts functions are ever called.

---

Comments from Reviewable

[cesarghali]

Review status: 0 of 31 files reviewed at latest revision, 1 unresolved discussion.

---

credentials/alts/utils.go, line 78 at r3 (raw file):

Previously, menghanl (Menghan Li) wrote…

I meant moving this to the same sync once for flag.Parse(). Because we don't need to do this if no alts functions are ever called.

Done.

---

Comments from Reviewable

[menghanl]

Review status: 0 of 31 files reviewed at latest revision, 1 unresolved discussion.

---

credentials/alts/alts.go, line 117 at r5 (raw file):

	once.Do(func() {
		flag.Parse()
		vmOnGCP = isRunningOnGCP()

Hmm, this is not thread safe... newALTS could be called in parallel.

---

Comments from Reviewable

[cesarghali]

Review status: 0 of 31 files reviewed at latest revision, 1 unresolved discussion.

---

credentials/alts/alts.go, line 117 at r5 (raw file):

Previously, menghanl (Menghan Li) wrote…

Hmm, this is not thread safe... newALTS could be called in parallel.

My understanding is that Do blocks if it's called multiple times and only the first time will trigger the function:
https://golang.org/src/sync/once.go?s=1137:1164#L25

---

Comments from Reviewable

[menghanl]

credentials/alts/alts.go, line 117 at r5 (raw file):

Previously, cesarghali (Cesar Ghali) wrote…

My understanding is that Do blocks if it's called multiple times and only the first time will trigger the function:
https://golang.org/src/sync/once.go?s=1137:1164#L25

OK, right. I missed that.

---

Comments from Reviewable

[dfawley]

One minor thing I noticed; otherwise LGTM.

[dfawley]

case a, b:
  return 1

or

case a,
  b:
  return 1

(if you like the way that looks better.)

[dfawley]

Delete this line and the one above; the case statements can be in one switch.

[cesarghali]

I don't even know how this code ended up with two switches :)
Fixed now.