Cloud Pub/Sub: authenticated push in GAE Standard for Python 3 #2097
Conversation
| token = bearer_token.split(' ')[1] | ||
| TOKENS.append(token) | ||
|
|
||
| header = jwt.decode_header(token) |
There was a problem hiding this comment.
Is this verifying the audience ('aud') field? Does it need to? This makes sure it is from pub/sub, but it could possibly be relayed from a push notification to a different app. Or do all push notifications use the same aud value?
There was a problem hiding this comment.
No, it is not verifying the aud field. It decodes the JWT header as shown. (If --push-auth-token-audience isn't set when creating the push subscription, then the aud field will be set to the push endpoint.)
I don't have code that specifically checks the aud field, should I add?
# JWT header
{"alg":"RS256","kid":"7d680d8c70d44e947133cbd499ebc1a61c3d5abc","typ":"JWT"}
# JWT claim
{
"aud":"https://example.com",
"azp":"113774264463038321964",
"email":"gae-gcp@appspot.gserviceaccount.com",
"sub":"113774264463038321964",
"email_verified":true,
"exp":1550185935,
"iat":1550182335,
"iss":"https://accounts.google.com"
}
There was a problem hiding this comment.
No, I don't think so. This all looks very good to me. Go ahead and merge when you're ready, or let me know if you want me to do it.
There was a problem hiding this comment.
@anguillanneuf I'm just getting started reading this, so apologies if you are doing this, but the rule of thumb on these things is to verify the signature before touching any of the fields, then look at the expires and then the audience. (in that order). Then everything else.
There was a problem hiding this comment.
@lesv 1. verifying signature and 2. checking expiration time are both done by the Google Auth Python library. I added 3. checking the audience field in the claim. Thanks for catching this!
There was a problem hiding this comment.
@anguillanneuf I found some time to review this. Did not actually run the example, just glanced over the code.
I see that this PR has already been approved in the meanwhile, thus if you feel the comments I gave are not significant enough to warrant a yet another review cycle, feel free to discard them. :)
Edit: I see that some of the comments have already been addressed in later commits - just ignore them, please (I was reviewing the PR commit by commit).
| token = bearer_token.split(' ')[1] | ||
| TOKENS.append(token) | ||
|
|
||
| header = jwt.decode_header(token) |
There was a problem hiding this comment.
@anguillanneuf I'm just getting started reading this, so apologies if you are doing this, but the rule of thumb on these things is to verify the signature before touching any of the fields, then look at the expires and then the audience. (in that order). Then everything else.
There was a problem hiding this comment.
Please fix README
Users should change the claim validation should also be called out in both the README and the code.
I'm going to click approve so that you can submit based on someone else's review, feel free to call me out specifically if you'd like me to review again.
TL;DR: Authenticated Push is a new Cloud Pub/Sub beta feature. After it has been set up as such, Cloud Pub/Sub generates a JWT and attaches it to the "Authorization" header in the push request. Users can verify the token using the Google Auth Python library to confirm that the push requests indeed come from Cloud Pub/Sub.
Things to check extra carefully in this PR:
main.py:receive_messages_handlermain_test.py:test_push_endpointI create a mock token using the private key in
data/privatekey.pem. I monkeypatched the function to verify oauth2 tokens so that it doesn't get checked against the real Google public certs but what's indata/public_cert.pem. Thesepemfiles are the same as those from the tests ingoogle-auth-library-python.