Access to TLS client certificate #111

Closed
tv42 opened this Issue Mar 10, 2015 · 43 comments

Projects

None yet

10 participants

@tv42
tv42 commented Mar 10, 2015

I can't see any way for an RPC method to authenticate a client based on a TLS certificate.

An example program where an RPC method echoes the client TLS certificate would be great.

@tv42
tv42 commented Mar 10, 2015

Don't know how well this aligns with your plans, but adding a Conn() net.Conn method to ServerTransport, with a trivial implementation in http2Server, is enough to do this, and also provides RemoteAddr etc.

@iamqizhao
Contributor

I would suggest adding a read-only accessor (e.g., Conn()) to
credentials.TransportAuthenticator instead.

On Tue, Mar 10, 2015 at 1:24 PM, Tv notifications@github.com wrote:

Don't know how well this aligns with your plans, but adding a Conn()
net.Conn method to ServerTransport, with a trivial implementation in
http2Server, is enough to do this, and also provides RemoteAddr etc.


Reply to this email directly or view it on GitHub
#111 (comment).

@iamqizhao
Contributor

Probably

credentials.TransportAuthenticator.ConnectionState() returns a struct similar to http://golang.org/pkg/crypto/tls/#ConnectionState.

@tv42
tv42 commented Mar 10, 2015

I don't see credentials.TransportAuthenticator interacting with the server side at all. At most it's used to wrap a net.Listener (with no place to stash extra data). The plan for its future is unclear. Is there something planned that is just not implemented yet?

I agree that exposing net.Conn feels dirty, but the alternative is a lot of case-by-case things (TLS ConnectionState! RemoteAddr! Client public key for a non-TLS protocol!). Needing to support every underlying transport explicitly sounds wrong.

Perhaps the really modular way is something like func (s *Server) ServeConn(ctx context.Context, conn net.Conn) error, that lets the caller pass whatever is needed in the context. This would do what one iteration of the Serve for { Accept() } loop does now.

If pb.RegisterXxxServer was done differently, I'd be happy creating the struct that's the receiver of all the RPC methods separately for every connection, instead of sharing the same value for all connections, and storing any state I want in there -- but it seems like Context is more where the world is heading. In my own explorations of RPC in http://godoc.org/github.com/tv42/birpc , imitating net/rpc, I separated Registry from Codec to make that nicer -- though I ended up implementing an API I don't personally like anymore, filling method arguments based on type with reflect.

@iamqizhao
Contributor

On Tue, Mar 10, 2015 at 4:20 PM, Tv notifications@github.com wrote:

I don't see credentials.TransportAuthenticator interacting with the
server side at all. At most it's used to wrap a net.Listener (with no
place to stash extra data). The plan for its future is unclear. Is there
something planned that is just not implemented yet?

https://github.com/grpc/grpc-go/blob/master/interop/server/server.go#L201

I agree that exposing net.Conn feels dirty, but the alternative is a lot
of case-by-case things (TLS ConnectionState! RemoteAddr! Client public key
for a non-TLS protocol!). Needing to support every underlying transport
explicitly sounds wrong.

I said something similar to tls.ConnectionState instead of a copy of it.
Plus, most of members are pretty common for various transport level
security protocols.

Perhaps the really modular way is something like func (s *Server)
ServeConn(ctx context.Context) error, that lets the caller pass whatever
is needed in the context. This would do what one iteration of the Serve for
{ Accept() } loop does now.

If pb.RegisterXxxServer was done differently, I'd be happy creating the
struct that's the receiver of all the RPC methods separately for every
connection, instead of sharing the same value for all connections, and
storing any state I want in there -- but it seems like Context is more
where the world is heading. In my own explorations of RPC in
http://godoc.org/github.com/tv42/birpc , imitating net/rpc, I separated
Registry from Codec to make that nicer -- though I ended up implementing an
API I don't personally like anymore, filling method arguments based on type
with reflect.


Reply to this email directly or view it on GitHub
#111 (comment).

@tv42
tv42 commented Mar 10, 2015

I still don't see how you want the RPC method to actually access the ConnectionState. RPC methods don't get passed the TransportAuthenticator. TransportAuthenticator is also not a per-connection value, so I don't see how adding methods to that is helpful -- except if they also take either net.Conn or Context as an argument.

If you're still talking about exposing net.Conn, then I personally don't even need anything more from TransportAuthenticator, I can hardcode my app to type assert to *tls.Conn etc.

If you're talking about passing Context to a TransportAuthenticator method to look up some sort of client credentials, then where does the TransportAuthenticator implementation get to do context.WithValue? Also, at that point, it might as well be a standalone function -- I'm not at all convinced the client credentials are all the same type, so the calling code already needs to know what it's asking for, and then can call therightpkg.AuthFromContext(ctx).

I'm really keen to know if you have a plan for all this, e.g. based on some Google internal service or Java code, that's just slowly getting written in Go..

@iamqizhao
Contributor

Hi Tv,

I think there was a miscommunication in the beginning. To clarify, your
intention is to let users be able to extract the security info (e.g.,
client auth cert) for each incoming RPC on the server? If it is the case,
this is a generic question for gRPC. Do you have a real use case for this
requirement? Inside google, we do need to export some security info per RPC
to users but this part has not been fleshed out.

On Tue, Mar 10, 2015 at 4:43 PM, Tv notifications@github.com wrote:

I still don't see how you want the RPC method to actually access the
ConnectionState. RPC methods don't get passed the TransportAuthenticator.
TransportAuthenticator is also not a per-connection value, so I don't see
how adding methods to that is helpful -- except if they also take either
net.Conn or Context as an argument.

If you're still talking about exposing net.Conn, then I personally don't
even need anything more from TransportAuthenticator, I can hardcode my
app to type assert to *tls.Conn etc.

If you're talking about passing Context to a TransportAuthenticator
method to look up some sort of client credentials, then where does the
TransportAuthenticator implementation get to do context.WithValue? Also,
at that point, it might as well be a standalone function -- I'm not at all
convinced the client credentials are all the same type, so the calling code
already needs to know what it's asking for, and then can call
therightpkg.AuthFromContext(ctx).

I'm really keen to know if you have a plan for all this, e.g. based on
some Google internal service or Java code, that's just slowly getting
written in Go..


Reply to this email directly or view it on GitHub
#111 (comment).

@tv42
tv42 commented Mar 11, 2015

My use case is this: I'm porting a peer-to-peer communication protocol to gRPC.

An ealier version did it by streaming protobufs both ways over a HTTP POST, with the URL path deciding which "method" to call. From that perspective, gRPC sounds pretty familiar, right? To me, gRPC seemed like a good way to avoid boilerplate or writing my own custom framework.

I identify which peer I'm talking to by the client certificate (and respectively, the peer verifies the server identity with the server cert). The RPC methods naturally need to know which exact peer is calling them.

@tv42
tv42 commented Mar 11, 2015

And just to state this explicitly one more time, my current needs would be 100% covered with either of these:

  1. func (s *Server) ServeConn(ctx context.Context, conn net.Conn) error, with a context derived from that being passed to the RPC method, would let me pass in whatever I need. I'd just do my own for-accept loop, and prepare a context with the information I'll need later.
  2. a Conn() net.Conn method in ServerTransport and http2Server would let me dig out what I need.

Of these, 1. seems prettier to me.

(I typoed option 1. earlier and left out the net.Conn, sorry about that; updated the comment above.)

@tv42
tv42 commented Mar 11, 2015

Example use of ServeConn:

for {
    c, err := l.Accept()
    if err != nil { ... }
    ctx := context.WithValue(context.Background(), myKey, metadata(c))
    go srv.ServeConn(ctx, c)
}
@acasajus

The default approach to this would be to be able to define a set of handlers execute before the actual RPC request that can modify the context to add whatever info we need. It would be nice to have a set of handlers for the connection and another for each stream.

In the set of handlers that execute when a new connection is received we could do authentication and stuff like that.
And in the set of handlers that execute when a new stream is created we could do authorization.

  1. Accept new connection
  2. Execute handlers for new connection. We can do authentication here. If the client is not valid just close the connection or something.
  3. Wait for streams
  4. For each new stream execute handlers. We can do authorization here. If the client is not allowed to execute whatever he wants to do, close the connection or something
  5. Execute whatever RPC method

Maybe the stream handlers can be defined in the service?

@tv42
tv42 commented Mar 11, 2015

@acasajus For 2., the authentication also needs to pass information to the RPC method, e.g. who the current user is. Making the function both take and return a Context would be enough.

@acasajus

@tv42 agreed 👍

@GeertJohan

This sounds pretty handy.
There is some overlap with the DialOptions and Credentials way of handling auth, I wonder how this will fit between those.

@acasajus

For handlers per connection there's no service yet defined so somehow we'd need to add some info to the context so the handler can check if the authentication credentials are valid.

@tv42
tv42 commented Mar 20, 2015

Here's a concrete proposal. Let me know what you think:

  1. Add a WithServerTransportAuthenticator ServerOption that takes a credentials.TransportAuthenticator and just remembers it in options.

  2. In Server.Serve, if a credentials.TransportAuthenticator was set with the above, wrap the listener with TransportAuthenticator.NewListener.

    This is optional and unrelated to the other changes, but this seems like the intended purpose of NewListener.

  3. Make http2Server.operateHeaders take a context argument, and pass it in from http2Server.HandleStreams.

  4. Make http2Server.HandleStreams take a context argument; change transport.ServerTransport. Make Server.Serve pass it in.

  5. Add a NewServerConn(ctx context.Context, conn net.Conn) (context.Context, error) method to credentials.TransportAuthenticator. Existing implementations can just pass the context through unchanged.

    In Server.Serve after a successful Accept, if a credentials.TransportAuthenticator was set, wrap the context with TransportAuthenticator.NewServerConn. On error, close the connection.

    This lets the TransportAuthenticator set things in context based on the connection information.

Optionally, to make access control even easier, let the TransportAuthenticator guard the method calls. Something like NewServerCall(ctx context.Context, call Xxx) (ctx context.Context, error), if that returns an error don't call the RPC method but just return error to client.

For example, TransportAuthenticator.NewServerCall implementations could just return grpc.Errorf(codes.Unauthenticated, "TLS client certificate expired"), or set a context deadline based on account policy.

I'm not 100% sure about the right arguments, your judgement is appreciated.

That last part might also tie into how a Dapper-like tracing system would plugin -- perhaps there should be something to see the end of the request, too?

@tv42
tv42 commented Mar 20, 2015

Maybe that error in 5. above should be just ok bool, as I don't see where the error message could go, at that stage. Or maybe make ctx==nil special? Your call. My prototype didn't actually include that error there, but I can see how closing the error could be desirable. What would happen if NewServerConn just said conn.Close()?

NewServerCall can report the error to the client, so there error makes a lot more sense.

@iamqizhao
Contributor

Thanks. Haven't got time reading through. will get back to you shortly.

On Thu, Mar 19, 2015 at 6:16 PM, Tv notifications@github.com wrote:

Maybe that error in 5. above should be just ok bool, as I don't see where
the error message could go, at that stage. Or maybe make ctx==nil
special? Your call. My prototype didn't actually include that error there,
but I can see how closing the error could be desirable. What would happen
if NewServerConn just said conn.Close()?

NewServerCall can report the error to the client, so there error makes a
lot more sense.


Reply to this email directly or view it on GitHub
#111 (comment).

@tv42 tv42 added a commit to bazil/grpc-go that referenced this issue Apr 1, 2015
@tv42 tv42 TransportAuthenticator gets to prepare context for every connection
fixes grpc#111
ac27e5f
@tv42 tv42 added a commit to bazil/bazil that referenced this issue Apr 1, 2015
@tv42 tv42 util/grpcunix: Adjust to gRPC proposed authentication change
This commit assumes a fork of gRPC with the changes described in
grpc/grpc-go#111 (comment)
Something like this should be enough to use the unofficial branch:

    go get -d google.golang.org/grpc
    cd "$(go list -f '{{.Dir}}' google.golang.org/grpc)"
    git checkout -b auth
    git pull https://github.com/bazillion/grpc-go auth
    go install
5802c88
@tv42 tv42 added a commit to bazil/bazil that referenced this issue Apr 1, 2015
@tv42 tv42 util/grpcedtls: EdTLS authentication for gRPC connections
This commit assumes a fork of gRPC with the changes described in
grpc/grpc-go#111 (comment)
Something like this should be enough to use the unofficial branch:

    go get -d google.golang.org/grpc
    cd "$(go list -f '{{.Dir}}' google.golang.org/grpc)"
    git checkout -b auth
    git pull https://github.com/bazillion/grpc-go auth
    go install
4dfe3b0
@psanford
Contributor

Any update on this?

@iamqizhao
Contributor

Given the recent change, users can write a custom TransportAuthenticator to
extract auth info on server side via
https://github.com/grpc/grpc-go/blob/master/credentials/credentials.go#L154.
The mapping from rpc to transport is still pending though.

On Thu, May 14, 2015 at 7:52 AM, Peter Sanford notifications@github.com
wrote:

Any update on this?


Reply to this email directly or view it on GitHub
#111 (comment).

@tv42 tv42 added a commit to bazil/grpc-go that referenced this issue May 18, 2015
@tv42 tv42 TransportAuthenticator gets to prepare context for every connection
fixes grpc#111
fb55583
@tv42 tv42 added a commit to bazil/grpc-go that referenced this issue May 18, 2015
@tv42 tv42 TransportAuthenticator gets to prepare context for every connection
fixes grpc#111
3ff306a
@psanford
Contributor

There is still no easy way to get certificate info inside a handler. What do you think of @tv42's patch for this: bazil@3ff306a?

@tv42
tv42 commented Jun 23, 2015

FWIW the numbered branches are historical, the one without the number is latest: https://github.com/bazil/grpc-go/commits/auth

@psanford psanford added a commit to retailnext/grpc-go that referenced this issue Jun 24, 2015
@tv42 @psanford tv42 + psanford TransportAuthenticator gets to prepare context for every connection
fixes grpc#111
a34965e
@iamqizhao
Contributor

This API is under design for the beta. I'll finalize the design ASAP once I
am back from vacation (7/17).

On Tue, Jun 23, 2015 at 4:21 PM, Tv notifications@github.com wrote:

FWIW the numbered branches are historical, the one without the number is
latest: https://github.com/bazil/grpc-go/commits/auth


Reply to this email directly or view it on GitHub
#111 (comment).

@psanford
Contributor

Awesome. Thanks for the update.

@tv42 tv42 added a commit to bazil/grpc-go that referenced this issue Jul 28, 2015
@tv42 tv42 TransportAuthenticator gets to prepare context for every connection
fixes grpc#111
4b82ca8
@andres-erbsen

Are there any updates on this?

@andres-erbsen andres-erbsen referenced this issue in yahoo/coname Aug 20, 2015
Closed

Verifier authentication #17

@iamqizhao
Contributor

working on it. Instead of giving the entire certificate, I would like to know which fields in tls.ConnectionState are needed typically? The plan is to return the required fields to server handler as part of metadata. Using entire certificate introduces a lot of problems because we do not know the exact type of the certificate.

@andres-erbsen

I'm glad to hear that you are working on it. Which fields of tls.ConnectionState should be passed through depends on the limitations you are willing to impose on the users of this library -- tls.ConnectionState is already a chosen subset of the connection parameters, selected to be presented to the application developers.

  • I think HandshakeComplete is always true when a RPC has been received, so it can be dropped.
  • I would also guess that NegotiatedProtocol is used to negotiate http2, and thus redundant as well.
  • If http2 handles ServerName already, it is probably safe to drop it from here. If not, it is required for virtual hosting.
  • VerifiedChains and PeerCertificates -- required for doing mandatory and optional authentication (respectively) using plaintext certificates.
  • TLSUnique is in a significant sense the correct way to externally authenticate TLS connections; not providing it will result in the users who need external authentication implementing ugly hacks and likely messing up security-wise.
  • OCSPResponse and SignedCertificateTimestamps -- for checking extended validation (the "green bar" in browsers). Short-lived certificates (OCSP) with transparency (SCT) are the future of TLS, and it would be a pity to keep from the users of this library.
  • I am not attached to the Version, DidResume, and CipherSuite fields; the only use case I can think of is requiring different TLS configurations for different RPCs...
@iamqizhao
Contributor

After talking to grpc c ppl, I am going to provide a minimal set of info which is consistent with what grpc c does right now in TLS transport authenticator. If it does not fit your requirements, it is trivial to provide your own custom authenticator impl to grpc.

@tamird
Contributor
tamird commented Aug 21, 2015

If it does not fit your requirements, it is trivial to provide your own custom authenticator impl to grpc.

How would one provide a custom authenticator impl?

@psanford
Contributor

A custom authenticator only solves the problem if it can pass information about the certificate on to the rpc endpoints. I don't know how you would do that with the current implementation.

@iamqizhao
Contributor

@psanford I will tune the authenticator API so that it can pass info about the cert in the coming PR.

@iamqizhao iamqizhao closed this in #302 Aug 24, 2015
@psanford
Contributor

It seems like the information is still not available to the rpc endpoints. From my brief investigation, it looks like the code in transport.newHPACKDecoder() that copies the metadata from d.mdata to d.state.mdata never gets called.

@iamqizhao
Contributor

You can add a print at
https://github.com/grpc/grpc-go/blob/master/test/end2end_test.go#L122 to
check whether there is an entry keyed with "transport_security_type" which
is from ServerHandshake. If you checked the client side, yes the current
ClientHandshake does not return any auth info.

On Tue, Aug 25, 2015 at 4:41 PM, Peter Sanford notifications@github.com
wrote:

It seems like the information is still not available to the rpc endpoints.
From my brief investigation, it looks like the code in
transport.newHPACKDecoder() that copies the metadata from d.mdata to
d.state.mdata never gets called.


Reply to this email directly or view it on GitHub
#111 (comment).

@iamqizhao
Contributor

#304 changes the way on how to access client auth info. Now it is not part of normal metadata. Check https://github.com/grpc/grpc-go/blob/master/test/end2end_test.go#L116 as an example on how to access auth info in service handler.

@psanford
Contributor

This looks great! Thanks for getting this merged.

@tamird
Contributor
tamird commented Sep 5, 2015

This still seems unusable for the TLS case. TLSInfo.state is unexported, how can any authentication be done based on information contained in it?

@psanford
Contributor
psanford commented Sep 6, 2015

You still have to implement your own TransportAuthenticator to get access to the certificates. Its probably easiest to make a new struct type that embeds a TransportAuthenticator and just overrides the ServerHandshake method.

@tamird
Contributor
tamird commented Sep 6, 2015

That seems wrong. What is the point of TLSInfo.state? It's not used internally and not exported.

EDIT: TransportAuthenticator is also the wrong level, because it precludes the possibility of per-method authentication unless you stash random state into your Context.

@iamqizhao
Contributor

It should be exported (I made a mistake when I switched among multiple
potential implementations in my pr). Thanks for your fix.

On Sun, Sep 6, 2015 at 9:49 AM, Tamir Duberstein notifications@github.com
wrote:

That seems wrong. What is the point of TLSInfo.state at all? It's not
used internally and not exported.


Reply to this email directly or view it on GitHub
#111 (comment).

@smithbk
smithbk commented Oct 23, 2016

So if I want access to the client certificate, what is the conclusion here? Do I have to write my own TransportAuthenticator and override the ServerHandshake method? If yes, is there an example?

It would be very useful to have a simple example of how to access the client certificate.

Thanks

@jcramb
jcramb commented Jan 27, 2017 edited

This is also something I require in my project -> after reading through this and some of the Credentials source code I've come up with the following. Would appreciate some feedback on whether this is the intended way to solve this problem or if there is something more elegant...

type custom struct {
    credentials.TransportCredentials
}

func (c *custom) ServerHandshake(conn net.Conn) (net.Conn, credentials.AuthInfo, error) { 
    conn, authInfo, err := c.TransportCredentials.ServerHandshake(conn)
    tlsInfo := authInfo.(credentials.TLSInfo)
    name := tlsInfo.State.PeerCertificates[0].Subject.CommonName
    fmt.Printf("%s\n", name)
    return conn, authInfo, err
}
@MakMukhi
Contributor
@jcramb
jcramb commented Jan 28, 2017 edited

@smithbk Actually for posterity, I've just discovered that the client certificate can also be accessed using the grpc Peer package to extract the Addr/AuthInfo for a client from within an RPC call.

func (s *server) HandleCommand(ctx context.Context, in *pb.CommandRequest) (*pb.CommandReply, error) {
    peer, ok := peer.FromContext(ctx)
    if ok {
        tlsInfo := peer.AuthInfo.(credentials.TLSInfo)
        v := tlsInfo.State.VerifiedChains[0][0].Subject.CommonName
        fmt.Printf("%v - %v\n", peer.Addr.String(), v)
    }
    return &pb.CommandReply{Out: "Cmd: " + in.Cmd}, nil
} 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment