Gdt 100 auth lambda #29
Conversation
Pull Request Test Coverage Report for Build 8381799908Details
💛 - Coveralls |
JPrevost
force-pushed
the
gdt-100-auth-lambda
branch
from
f4e2f55
to
ee2ac9d
Compare
There was a problem hiding this comment.
Overall, looks good to me!
Feel like the organization of code and functionality matches the relative simplicity of the lambda itself (e.g. pondered if a standalone function might be helpful for constructing the redirect URL, but didn't feel necessary).
Before approval, left a couple of questions, with the two that gave me approval pause being:
- potential logging of SSM params
- consider throwing explicit exception if SSM params not set
lambdas/lambda_edge.py
Outdated
| Path="/apps/cf-lambda-geo-auth/", WithDecryption=False | ||
| ) | ||
|
|
||
| logging.info(ssm_params) |
There was a problem hiding this comment.
Do we have concerns with logging SSM parameters? Looks as though one parameter is the JWT secret which could be sensitive.
There was a problem hiding this comment.
This was a leftover debug log I had dropped in and forgot to remove. Technically anyone that can read the logs can probably also read the SSM param in the console so it's only a pseudo-secret but I do agree it'll be better to remove this line.
| def parse_cookies(headers): | ||
| parsed_cookie = {} | ||
| if headers.get("cookie"): | ||
| for cookie in headers["cookie"][0]["value"].split(";"): |
There was a problem hiding this comment.
The Lambda@Edge docs don't seem to have an example, but is it possible there could be multiple cookies in this headers["cookie"] array? If so, are we safe just grabbing the 0th one?
The AWS "Q" chat thing suggests they could look like:
[
{
"key": "cookie1",
"value": "cookie1Value"
},
{
"key": "cookie2",
"value": "cookie2Value"
}
]And/or, do we know the name of the cookie and could use the key property to find it?
There was a problem hiding this comment.
Great question. I'll see if I can find more info on real-world scenarios on what to expect here.
There was a problem hiding this comment.
I've opened a separate ticket to investigate and update code if appropriate for the cookie array
| jwt_secret = param["Value"] | ||
| if param["Name"] == "/apps/cf-lambda-geo-auth/auth-url": | ||
| auth_url = param["Value"] | ||
|
|
There was a problem hiding this comment.
Wonder if we should throw an exception here if not all the expected SSM params are found?
The advantage I see would be easy debugging if the lambda and TF code got out of sync, where otherwise we'd just get errors about undefined variables.
| "host": [{"key": "Host", "value": "d123.cf.net"}], | ||
| "cookie": [ | ||
| { | ||
| "key": "Cookie", | ||
| "value": f"mitlcdnauthjwt={valid_token}; AnotherOne=A; X-Name=B", | ||
| } | ||
| ], |
There was a problem hiding this comment.
Maybe this fixture answers my question about the 0th cookie above... looks like all the header dictionary values are lists? If so, makes sense that it could be safe to the take the 0th one.
There was a problem hiding this comment.
Yeah it's definitely odd as it's an array but it feels like it doesn't function as one. I'll dig in more on this as it should be fairly easy for me to set a bunch of cookies locally and see how they get treated.
There was a problem hiding this comment.
I've opened a separate ticket to investigate and update code if appropriate for the cookie array
| """ | ||
| request = event["Records"][0]["cf"]["request"] | ||
| headers = request["headers"] | ||
| jwt_secret, auth_url = load_ssm_params() |
There was a problem hiding this comment.
Think that was a good call to break out into another function.
| if jwt_secret is None: | ||
| msg = "/apps/cf-lambda-geo-auth/jwt-secret not found in parameter store" | ||
| logging.exception(msg) | ||
| raise RuntimeError(msg) | ||
|
|
||
| if auth_url is None: | ||
| msg = "/apps/cf-lambda-geo-auth/auth-url not found in parameter store" | ||
| logging.exception(msg) | ||
| raise RuntimeError(msg) |
Why are these changes being introduced: * This adds the logic to check for JWT tokens and redirect if the token is missing, invalid, or expired * This follows the build process expected by our infrastructure code Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/GDT-100
Why are these changes being introduced: * Code review suggested we throw exceptions if the parameters were not found in SSM. This led to more complexity in the code block which felt like it was easier to understand when extracted to it's own method. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/GDT-100 Document any side effects to this change: * Some method documentation has been updated to better describe the core logic of the lambda
JPrevost
force-pushed
the
gdt-100-auth-lambda
branch
from
a92cd71
to
cf8f9c1
Compare
Why are these changes being introduced:
is missing, invalid, or expired
Relevant ticket(s):
Includes new or updated dependencies?
YES
Developer
Dev CF Lambda@Edge Full Deployworkflow has been run to update CloudFront in Dev1Code Reviewer(s)