Add API authentication mechanism #155
Conversation
cmd/soapboxd/main.go
Outdated
| handler grpc.UnaryHandler, | ||
| ) (interface{}, error) { | ||
| if md, ok := metadata.FromContext(ctx); ok { | ||
| if _, ok := md["skip_auth"]; ok { |
There was a problem hiding this comment.
I think fundamentally, we can't let the client be the one deciding whether an RPC requires auth or not -- it would be too trivial for a malicious client to bypass auth on sensitive calls.
I think instead, each RPC implementation on the server-side should indicate somehow that it requires auth. Then the interceptor here would test to see if a given handler required auth, and then would call authorize.
Off the top of my head, I'm not sure what the best way is to do this. One challenge is that the methods have different signatures, so a naive approach of creating a type that defined them, and then mapping from that type to a bool (whether auth is required), would not be straightforward. You could do a map of interface{} to bool, type-switching on each method, but this could be very verbose. I think we need to think about how to do this before proceeding. There are probably a couple of different approaches. I am happy to help think through this.
There was a problem hiding this comment.
Ahh, that is a great point. I think I was just trying to get something working, but we definitely need to have the server deciding when to auth rather than the client. Let me see if I can get something working here, if I get stuck I'll reach out.
There was a problem hiding this comment.
There are only two server methods that don't need to be authed, namely LoginUser and CreateUser, so if we can read the method names in the interceptor somehow, we can just skip for those methods. Not sure how to read which method is being called from there though, is it listed in the Context?
There was a problem hiding this comment.
The info param to that function, of type UnaryServerInfo, is a struct type that has a field FullMethod which is the full RPC method string -- you could do some pattern-matching on that.
kalilsn
dismissed
paulsmith’s stale review
Nick made the requested change (server decides which methods require auth)
kalilsn
requested review from
orenfromberg and
robertfairhead
and removed request for
isawpalmetto
| } | ||
| key := []byte(os.Getenv("LOGIN_SECRET_KEY")) | ||
| h := hmac.New(sha512.New, key) | ||
| h.Write(userID) |
There was a problem hiding this comment.
If i understand this correctly, the user token is essentially static because Key + UserID should always result in the same token.
This isn't wonderful because it doesn't tie the authorization token to a specific session and it's not revocable. If somebody ever got the token value they could impersonate the user all day long.
Not an immediately critical thing to fix since some authorization is a step forward. But a thing to return to later.
There was a problem hiding this comment.
Yeah, this doesn't seem great. Stealing this token would let you impersonate the user literally forever. I'll throw in a TODO.
Adding some randomness to the token isn't hard, but if we do that, we have to store it. So perhaps in the future we should put it in the database, tied to the user, along with a timestamp so it will expire automatically?
AFAIK soapboxd doesn't really have the concept of a session yet, so I'm very open to suggestions here.
There was a problem hiding this comment.
👍
The revocable bit is probably more important (since that's how most API tokens work for command-line etc.)
The session scoping would only make sense if we got rid of the CLI and went web-app only since then you could do an Oauth flow to get a JWT and present that.
Some authentication is better than none so may as well ship this and revisit.
soapboxd/user.go
Outdated
| @@ -91,19 +96,21 @@ func (s *server) LoginUser(ctx context.Context, req *pb.LoginUserRequest) (*pb.L | |||
| Error: genericLoginErrorMsg, | |||
| } | |||
|
|
|||
| user, err := s.getUserByEmail(ctx, req.GetEmail()) | |||
| if err == sql.ErrNoRows { | |||
| user, getUserErr := s.getUserByEmail(ctx, req.GetEmail()) | |||
There was a problem hiding this comment.
i'm unclear why this doen't use err. It's not wrong, just not idiomatic to use anything other than err.
|
I tried to see if it was possible to eliminate the need for manually passing the user metadata with each request using an interceptor but couldn't figure out how to get the session data in my interceptor, so I'm just going to merge this. Might revisit that part later. |
Fixes #133
I'm not crazy about having to pass the user metadata every time an API method is called from Rails, so I may try to figure out a better way to do that, but I'd like some initial feedback on the approach.