Skip to content

Expose the request context in the FPM layer via a global variable#501

Closed
pl4yradam wants to merge 52 commits intobrefphp:masterfrom
pl4yradam:pl4yradam-patch-1
Closed

Expose the request context in the FPM layer via a global variable#501
pl4yradam wants to merge 52 commits intobrefphp:masterfrom
pl4yradam:pl4yradam-patch-1

Conversation

@pl4yradam
Copy link
Contributor

@pl4yradam pl4yradam commented Nov 10, 2019

This request context is needed if a user is using the authentication type of IAM_ROLE, when a user authenticates.

When using the IAM_ROLE authentication type with serverless, a user will pass their v4 signature token to API Gateway. API Gateway will then check and authorise the user which then invokes the lambda function, when invoking the lambda function API Gateway will pass in an updated requestContext object which contains Cognito information about the user. This prevents the requirement of having to pass user ID's publicly.

When using the layer without fpm & using the lambda function you can retrieve the event.

Due to how FPM creates a local web server, handling the event is not accessible with the lambda function provided by bref. As the lambda function getting the LambdaApi request throws a no access error.

See below the example of the request context below contains information about the Cognito user, however in the fpmphp file there has unfortunately been no object added to this. I have written a pull request and adjusted a previous test that actually contained a requestContext but hadn't been tested to account for it.

requestContext": {
"resourceId": "n52n1n689a",
"resourcePath": "/",
"httpMethod": "GET",
"extendedRequestId": "C9gIXGW7joEFVxA=",
"requestTime": "10/Nov/2019:20:59:33 +0000",
"path": "/users",
"accountId": "",
"protocol": "HTTP/1.1",
"stage": "dev",
"domainPrefix": "secure-api",
"requestTimeEpoch": 1573419573440,
"requestId": "d679114c-4a3d-42d8-862a-892d036f7e2f",
"identity": {
"cognitoIdentityPoolId": "",
"accountId": "",
"cognitoIdentityId": "",
"caller": "",
"sourceIp": "",
"principalOrgId": "",
"accessKey": "",
"cognitoAuthenticationType": "authenticated",
"cognitoAuthenticationProvider": "",
"userArn": "",
"userAgent": "",
"user": ""
},
"domainName": "",
"apiId": ""
},`

pl4yradam and others added 13 commits November 10, 2019 21:00
This request context is needed if a user is using the authentication type of IAM_ROLE, when a user auths through Api Gateway it will then authorise the user and pass their information (securely as request context). At the moment the file misses this so a php function cant use the authorised details correctly.
Fixed whitespace
@mnapoli mnapoli changed the title Updated FPM php to cater for api gateways authorisers Expose the event context in the FPM layer via a global variable Nov 11, 2019
@mnapoli mnapoli changed the title Expose the event context in the FPM layer via a global variable Expose the request context in the FPM layer via a global variable Nov 11, 2019
@mnapoli
Copy link
Member

mnapoli commented Nov 11, 2019

I see, that addition makes sense, thanks for working on that!

One thing that I am wondering about: could the global variable $_SERVER['REQUEST_CONTEXT'] contain the array instead of JSON? Have you tried?

That would be simpler than having to decode JSON in PHP, which isn't really consistent with how global FPM variables work today.

@pl4yradam
Copy link
Contributor Author

leaving it to there now, attempted multiple way to change to array however unsuccessfull

@mnapoli
Copy link
Member

mnapoli commented Nov 18, 2019

I really want to make this happen. I see that now you populate a global $_SERVER key for each variable. I am wary about that solution:

  • since we populate variables dynamically we have no control over, it is always a "warning" flag for me (regarding security and things we didn't anticipate)
  • the list of variables available is not discoverable easily, and it is not consistent with the AWS documentation available online

I think we need to find a different solution. The JSON one seemed like a better option TBH. I wish we could find a way to make the array available in PHP 🤔 FastCGI does this with POST data, do we know if PHP has special handling for this global variable?

@pl4yradam
Copy link
Contributor Author

Okay i've spent more time on this solution now than I would like, The correct interpretation would be to pass the json through as is and use json_decode(). The second solution which was proposed by the author @mnapoli you wanted it to be under server variables which again I have catered for to now go back to the json idea?

I have forked and implemented my fix for my own work at the moment, people are welcome to use the code I have presented.

If you wish for me to further develop this on the short free time I have I am welcome for sponsors to contribute.

It may also be worth documenting in the bref docs the limitations of using FPM and the lambda function solo.

@adamprescott
Copy link

adamprescott commented Nov 20, 2019

I've been playing about with Bref for work to see if it's viable. I came across the same issue that this attempts to solve.

As for my opinion which would be the more elegant solution. FPM is kind of being used as a crutch anyway to transform execution into a web context. Passing the payload as JSON is probably the best we're going to get, unless there's another feature within FPM that can be used.

@mnapoli
Copy link
Member

mnapoli commented Nov 21, 2019

Hi, just to clarify, this is the ideal solution I described (maybe I wasn't clear enough sorry):

$id = $_SERVER['REQUEST_CONTEXT']['cognitoIdentityId'];

@pl4yradam
Copy link
Contributor Author

pl4yradam commented Nov 21, 2019

You will need to do this with all of the returned gateway response keys also. see the very first post, It wouldn't be a good idea to pick from the response your code will end up bloated and inefficient. If there are security concerns regarding what can be passed through in a request context thats probably another issue. As I agree with @adamprescott it seems fpm is more a crutch to the process.

@mnapoli
Copy link
Member

mnapoli commented Jan 28, 2020

Thanks, a rebased version of this PR has been merged in #542

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants