Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

redefine IoT Core custom authorizer request and response structs #401

Merged
merged 5 commits into from May 19, 2022
Merged

redefine IoT Core custom authorizer request and response structs #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
56107fd
redefine IoT Core custom authorizer request and response structs as p…
rittneje Oct 13, 2021
389713b
make IAM policy structs reusable
rittneje Oct 18, 2021
e518605
Merge branch 'main' into fix-iot-core-authorizer-request
bmoffatt May 18, 2022
6e68d8a
gofmt
May 18, 2022
ac94a86
restore deleted structs, add deprecation notices
May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 6 additions & 13 deletions events/apigw.go
View file
Open in desktop
Expand Up @@ -336,21 +336,14 @@ type APIGatewayV2CustomAuthorizerSimpleResponse struct {
Context map[string]interface{} `json:"context,omitempty"`
}

// APIGatewayCustomAuthorizerPolicy represents an IAM policy.
//
// Note: This type exists for backwards compatibility.
// should reference IAMPolicyDocument directly instead.
type APIGatewayCustomAuthorizerPolicy IAMPolicyDocument
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep backcompat, I believe this would need to be a type alias. eg: write as type APIGatewayCustomAuthorizerPolicy = IAMPolicyDocument

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not necessary for backwards compatibility. type A B makes A a copy of B (same struct fields), they just are not equivalent/interchangeable. type A = B makes A an alias of B, meaning they are interchangeable (e.g., type byte = uint8). Unless you want clients to be able to use an APIGatewayCustomAuthorizerPolicy wherever an IAMPolicyDocument is expected without casting, there is no need for an alias.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my brain read the diff as having APIGatewayCustomAuthorizerResponse also being updated to use IAMPolicyDocument for it's PolicyDocument field, which would have broken something like x.PolicyDocument = APIGatewayCustomAuthorizerPolicy{} (see: https://play.golang.org/p/_OXhN9ebJ1K). I see now that APIGatewayCustomAuthorizerResponse wasn't updated. I can't tell if that was intentional or not?

If you think that APIGatewayCustomAuthorizerResponse shouldn't be updated, then lets remove the //Deprecated: comment, as it's use of APIGatewayCustomAuthorizerPolicy would still be correct in that context

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "deprecated" comment was directed more at API developers (to tell them not to use it in new structs), not API clients.
I had not intended to make changes to APIGatewayCustomAuthorizerResponse but I can do that if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think to just drop the comment so that no one's linter freaks out. DRYing the rest api gateway type feels out of scope to me for this PR


type APIGatewayV2CustomAuthorizerIAMPolicyResponse struct {
PrincipalID string `json:"principalId"`
PolicyDocument APIGatewayCustomAuthorizerPolicy `json:"policyDocument"`
Context map[string]interface{} `json:"context,omitempty"`
}

// APIGatewayCustomAuthorizerPolicy represents an IAM policy
type APIGatewayCustomAuthorizerPolicy struct {
Version string
Statement []IAMPolicyStatement
}

// IAMPolicyStatement represents one statement from IAM policy with action, effect and resource
type IAMPolicyStatement struct {
Action []string
Effect string
Resource []string
}
14 changes: 14 additions & 0 deletions events/iam.go
View file
Open in desktop
@@ -0,0 +1,14 @@
package events

// IAMPolicyDocument represents an IAM policy document.
type IAMPolicyDocument struct {
Version string
Statement []IAMPolicyStatement
}

// IAMPolicyStatement represents one statement from IAM policy with action, effect and resource.
type IAMPolicyStatement struct {
Action []string
Effect string
Resource []string
}
48 changes: 30 additions & 18 deletions events/iot.go
View file
Open in desktop
@@ -1,34 +1,46 @@
package events

// IoTCustomAuthorizerRequest contains data coming in to a custom IoT device gateway authorizer function.
type IoTCustomAuthorizerRequest struct {
HTTPContext *IoTHTTPContext `json:"httpContext,omitempty"`
MQTTContext *IoTMQTTContext `json:"mqttContext,omitempty"`
TLSContext *IoTTLSContext `json:"tlsContext,omitempty"`
AuthorizationToken string `json:"token"`
TokenSignature string `json:"tokenSignature"`
// IoTCoreCustomAuthorizerRequest represents the request to an IoT Core custom authorizer.
// See https://docs.aws.amazon.com/iot/latest/developerguide/config-custom-auth.html
type IoTCoreCustomAuthorizerRequest struct {
Token string `json:"token"`
SignatureVerified bool `json:"signatureVerified"`
Protocols []string `json:"protocols"`
ProtocolData *IoTCoreProtocolData `json:"protocolData,omitempty"`
ConnectionMetadata *IoTCoreConnectionMetadata `json:"connectionMetadata,omitempty"`
}

type IoTHTTPContext struct {
type IoTCoreProtocolData struct {
TLS *IoTCoreTLSContext `json:"tls,omitempty"`
HTTP *IoTCoreHTTPContext `json:"http,omitempty"`
MQTT *IoTCoreMQTTContext `json:"mqtt,omitempty"`
}

type IoTCoreTLSContext struct {
ServerName string `json:"serverName"`
}

type IoTCoreHTTPContext struct {
Headers map[string]string `json:"headers,omitempty"`
QueryString string `json:"queryString"`
}

type IoTMQTTContext struct {
type IoTCoreMQTTContext struct {
ClientID string `json:"clientId"`
Password []byte `json:"password"`
Username string `json:"username"`
}

type IoTTLSContext struct {
ServerName string `json:"serverName"`
type IoTCoreConnectionMetadata struct {
ID string `json:"id"`
}

// IoTCustomAuthorizerResponse represents the expected format of an IoT device gateway authorization response.
type IoTCustomAuthorizerResponse struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the rename. To keep strict compile time backwards compatibility, I'd like for the old types to be left in the project too. They can have a comment like // Deprecated does not model schema correctly, use XYZType instead. They might also be moved to a file like iot_deprecated.go to help with the clutter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not put loadbearing invariants in comments please, as they are easy to overlook.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmoffatt Is it really necessary to keep the original struct? No one could be using it in practice since the PolicyDocuments field had the wrong type, and without that you won't have a particularly useful authorizer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wanna keep the old ones, type changes and deletions are breaking changes. This isn't the first time things have been gotten wrong, and so far this project has not respond to that by deleting code. I'm not satisfied with incorrect models hanging around either, and i'd like to try and solve that holistically rather than making possibly inconsistent actions on a case-by-case basis

@LegNeato

Let's not put loadbearing invariants in comments please, as they are easy to overlook.

The guidance comes from https://go.dev/blog/godoc

To signal that an identifier should not be used, add a paragraph to its doc comment that begins with “Deprecated:” followed by some information about the deprecation.

My IDE renders these with a helpful strikethrough. The form can also be captured by the popular staticcheck linter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd like to try and solve that holistically rather than making possibly inconsistent actions on a case-by-case basis

Really I think this library should have made a separate module for each service. That way clients can just include the events they need rather than using a single ever-growing events package. This would also reduce some of the repetition in the typedefs (e.g., iotcore.CustomAuthorizerResponse vs. events.IoTCoreCustomAuthorizerResponse), and would allow the event structs for each service to be versioned independently.

For example in this situation we could have just made a new major rev of the IoT Core events module rather than having to (ultimately pointlessly) maintain backwards compatibility.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I think it highlights some obvious shortcomings of the approach this library went with. I'll forward this on to the product team.

The approach you described is actually how it was just before the initial public release. The monolithic events package approached was chosen after receiving feedback from our private preview customers around poor event type discovery with IDE auto-complete suggestions at the time.

IsAuthenticated bool `json:"isAuthenticated"`
PrincipalID string `json:"principalId"`
DisconnectAfterInSeconds int32 `json:"disconnectAfterInSeconds"`
RefreshAfterInSeconds int32 `json:"refreshAfterInSeconds"`
PolicyDocuments []string `json:"policyDocuments"`
// IoTCoreCustomAuthorizerResponse represents the response from an IoT Core custom authorizer.
// See https://docs.aws.amazon.com/iot/latest/developerguide/config-custom-auth.html
type IoTCoreCustomAuthorizerResponse struct {
IsAuthenticated bool `json:"isAuthenticated"`
PrincipalID string `json:"principalId"`
DisconnectAfterInSeconds uint32 `json:"disconnectAfterInSeconds"`
RefreshAfterInSeconds uint32 `json:"refreshAfterInSeconds"`
PolicyDocuments []*IAMPolicyDocument `json:"policyDocuments"`
}
30 changes: 30 additions & 0 deletions events/iot_deprecated.go
View file
Open in desktop
@@ -0,0 +1,30 @@
package events

// IoTCustomAuthorizerRequest contains data coming in to a custom IoT device gateway authorizer function.
// Deprecated: Use IoTCoreCustomAuthorizerRequest instead. IoTCustomAuthorizerRequest does not correctly model the request schema
type IoTCustomAuthorizerRequest struct {
HTTPContext *IoTHTTPContext `json:"httpContext,omitempty"`
MQTTContext *IoTMQTTContext `json:"mqttContext,omitempty"`
TLSContext *IoTTLSContext `json:"tlsContext,omitempty"`
AuthorizationToken string `json:"token"`
TokenSignature string `json:"tokenSignature"`
}

// Deprecated: Use IoTCoreHTTPContext
type IoTHTTPContext IoTCoreHTTPContext

// Deprecated: Use IoTCoreMQTTContext
type IoTMQTTContext IoTCoreMQTTContext

// Deprecated: Use IotCoreTLSContext
type IoTTLSContext IoTCoreTLSContext

// IoTCustomAuthorizerResponse represents the expected format of an IoT device gateway authorization response.
// Deprecated: Use IoTCoreCustomAuthorizerResponse. IoTCustomAuthorizerResponse does not correctly model the response schema.
type IoTCustomAuthorizerResponse struct {
IsAuthenticated bool `json:"isAuthenticated"`
PrincipalID string `json:"principalId"`
DisconnectAfterInSeconds int32 `json:"disconnectAfterInSeconds"`
RefreshAfterInSeconds int32 `json:"refreshAfterInSeconds"`
PolicyDocuments []string `json:"policyDocuments"`
}
16 changes: 8 additions & 8 deletions events/iot_test.go
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ import (
"github.com/aws/aws-lambda-go/events/test"
)

func TestIoTCustomAuthorizerRequestMarshaling(t *testing.T) {
func TestIoTCoreCustomAuthorizerRequestMarshaling(t *testing.T) {

// read json from file
inputJSON, err := ioutil.ReadFile("./testdata/iot-custom-auth-request.json")
Expand All @@ -17,7 +17,7 @@ func TestIoTCustomAuthorizerRequestMarshaling(t *testing.T) {
}

// de-serialize into Go object
var inputEvent IoTCustomAuthorizerRequest
var inputEvent IoTCoreCustomAuthorizerRequest
if err := json.Unmarshal(inputJSON, &inputEvent); err != nil {
t.Errorf("could not unmarshal event. details: %v", err)
}
Expand All @@ -31,11 +31,11 @@ func TestIoTCustomAuthorizerRequestMarshaling(t *testing.T) {
test.AssertJsonsEqual(t, inputJSON, outputJSON)
}

func TestIoTCustomAuthorizerRequestMalformedJson(t *testing.T) {
test.TestMalformedJson(t, IoTCustomAuthorizerRequest{})
func TestIoTCoreCustomAuthorizerRequestMalformedJson(t *testing.T) {
test.TestMalformedJson(t, IoTCoreCustomAuthorizerRequest{})
}

func TestIoTCustomAuthorizerResponseMarshaling(t *testing.T) {
func TestIoTCoreCustomAuthorizerResponseMarshaling(t *testing.T) {

// read json from file
inputJSON, err := ioutil.ReadFile("./testdata/iot-custom-auth-response.json")
Expand All @@ -44,7 +44,7 @@ func TestIoTCustomAuthorizerResponseMarshaling(t *testing.T) {
}

// de-serialize into Go object
var inputEvent IoTCustomAuthorizerResponse
var inputEvent IoTCoreCustomAuthorizerResponse
if err := json.Unmarshal(inputJSON, &inputEvent); err != nil {
t.Errorf("could not unmarshal event. details: %v", err)
}
Expand All @@ -58,6 +58,6 @@ func TestIoTCustomAuthorizerResponseMarshaling(t *testing.T) {
test.AssertJsonsEqual(t, inputJSON, outputJSON)
}

func TestIoTCustomAuthorizerResponseMalformedJson(t *testing.T) {
test.TestMalformedJson(t, IoTCustomAuthorizerResponse{})
func TestIoTCoreCustomAuthorizerResponseMalformedJson(t *testing.T) {
test.TestMalformedJson(t, IoTCoreCustomAuthorizerResponse{})
}
36 changes: 21 additions & 15 deletions events/testdata/iot-custom-auth-request.json
View file
Open in desktop
@@ -1,18 +1,24 @@
{
"httpContext": {
"headers": {
"Accept-Language" : "en"
"token" :"aToken",
"signatureVerified": true,
"protocols": ["tls", "http", "mqtt"],
"protocolData": {
"tls" : {
"serverName": "serverName"
},
"queryString": "abc"
},
"mqttContext": {
"clientId": "someclient",
"password": "aslkfjwoeiuwekrujwlrueowieurowieurowiuerwleuroiwueroiwueroiuweoriuweoriuwoeiruwoeiur",
"username": "thebestuser"
},
"tlsContext": {
"serverName": "server.stuff.com"
"http": {
"headers": {
"X-Request-ID": "abc123"
},
"queryString": "?foo=bar"
},
"mqtt": {
"username": "myUserName",
"password": "bXlQYXNzd29yZA==",
"clientId": "myClientId"
}
},
"token": "someToken",
"tokenSignature": "somelongtokensignature"
}
"connectionMetadata": {
"id": "e56f08c3-c559-490f-aa9f-7e8427d0f57b"
}
}
13 changes: 11 additions & 2 deletions events/testdata/iot-custom-auth-response.json
View file
Open in desktop
Expand Up @@ -4,6 +4,15 @@
"disconnectAfterInSeconds": 86400,
"refreshAfterInSeconds": 300,
"policyDocuments": [
"{ \"Version\": \"2012-10-17\", \"Statement\": [ { \"Action\": [\"iot:Subscribe\"], \"Effect\": \"Allow\", \"Resource\": [\"*\"] } ] }"
{
"Version": "2012-10-17",
"Statement": [
{
"Action": ["iot:Publish"],
"Effect": "Allow",
"Resource": ["arn:aws:iot:us-east-1:<your_aws_account_id>:topic/customauthtesting"]
}
]
}
]
}
}