-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Add AWS Marketplace Entitlement verification #1619
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
255a2fa
Add AWS Marketplace Entitlement verification
lucacome e93855b
PR feedback
lucacome e1f60f0
Use more secure random string, update errors
lucacome 386bc5c
Update validate time function
lucacome 6a71280
unexport claims
lucacome aa47412
add timeout
lucacome 3d4c3e1
Remove log on success
lucacome 682cdcc
PR feedback
lucacome File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| // +build aws | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "context" | ||
| "crypto/rand" | ||
| "encoding/base64" | ||
| "errors" | ||
| "fmt" | ||
| "math/big" | ||
| "time" | ||
|
|
||
| "github.com/aws/aws-sdk-go-v2/config" | ||
| "github.com/aws/aws-sdk-go-v2/service/marketplacemetering" | ||
| "github.com/aws/aws-sdk-go-v2/service/marketplacemetering/types" | ||
|
|
||
| "github.com/dgrijalva/jwt-go/v4" | ||
| ) | ||
|
|
||
| var ( | ||
| productCode string | ||
| pubKeyVersion int32 = 1 | ||
| pubKeyString string | ||
| nonce string | ||
| ) | ||
|
|
||
| func init() { | ||
| startupCheckFn = checkAWSEntitlement | ||
| } | ||
|
|
||
| func checkAWSEntitlement() error { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| nonce, err := generateRandomString(255) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| cfg, err := config.LoadDefaultConfig(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("error loading AWS configuration: %v", err) | ||
| } | ||
|
|
||
| mpm := marketplacemetering.New(marketplacemetering.Options{Region: cfg.Region, Credentials: cfg.Credentials}) | ||
|
|
||
| out, err := mpm.RegisterUsage(ctx, &marketplacemetering.RegisterUsageInput{ProductCode: &productCode, PublicKeyVersion: &pubKeyVersion, Nonce: &nonce}) | ||
| if err != nil { | ||
| var notEnt *types.CustomerNotEntitledException | ||
| if errors.As(err, ¬Ent) { | ||
lucacome marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return fmt.Errorf("user not entitled, code: %v, message: %v, fault: %v", notEnt.ErrorCode(), notEnt.ErrorMessage(), notEnt.ErrorFault().String()) | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| pk, err := base64.StdEncoding.DecodeString(pubKeyString) | ||
| if err != nil { | ||
| return fmt.Errorf("error decoding Public Key string: %v", err) | ||
| } | ||
| pubKey, err := jwt.ParseRSAPublicKeyFromPEM(pk) | ||
| if err != nil { | ||
| return fmt.Errorf("error parsing Public Key: %v", err) | ||
| } | ||
|
|
||
| token, err := jwt.ParseWithClaims(*out.Signature, &claims{}, jwt.KnownKeyfunc(jwt.SigningMethodPS256, pubKey)) | ||
| if err != nil { | ||
| return fmt.Errorf("error parsing the JWT token: %v", err) | ||
| } | ||
|
|
||
| if claims, ok := token.Claims.(*claims); ok && token.Valid { | ||
| if claims.ProductCode != productCode || claims.PublicKeyVersion != pubKeyVersion || claims.Nonce != nonce { | ||
| return fmt.Errorf("the claims in the JWT token don't match the request") | ||
| } | ||
| } else { | ||
| return fmt.Errorf("something is wrong with the JWT token") | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| type claims struct { | ||
| ProductCode string `json:"productCode,omitempty"` | ||
| PublicKeyVersion int32 `json:"publicKeyVersion,omitempty"` | ||
| IssuedAt *jwt.Time `json:"iat,omitempty"` | ||
| Nonce string `json:"nonce,omitempty"` | ||
| } | ||
|
|
||
| func (c claims) Valid(h *jwt.ValidationHelper) error { | ||
| if c.Nonce == "" { | ||
| return &jwt.InvalidClaimsError{Message: "the JWT token doesn't include the Nonce"} | ||
| } | ||
| if c.ProductCode == "" { | ||
| return &jwt.InvalidClaimsError{Message: "the JWT token doesn't include the ProductCode"} | ||
| } | ||
| if c.PublicKeyVersion == 0 { | ||
| return &jwt.InvalidClaimsError{Message: "the JWT token doesn't include the PublicKeyVersion"} | ||
| } | ||
| if err := h.ValidateNotBefore(c.IssuedAt); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func generateRandomString(n int) (string, error) { | ||
| const letters = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-" | ||
| ret := make([]byte, n) | ||
| for i := 0; i < n; i++ { | ||
| num, err := rand.Int(rand.Reader, big.NewInt(int64(len(letters)))) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| ret[i] = letters[num.Int64()] | ||
| } | ||
|
|
||
| return string(ret), nil | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now this function is called every time the binary is executed. However, the binary can be executed without the intention of starting the IC - for example, a troubleshooting
/nginx-ingress versioncommand to determine the IC version. With the currentinit()implementation, that troubleshooting command with also register usage.However, there is a bit bigger concern I see - the function has a lot of logic which we don't test at all.
To address those concerns I suggest the following:
(1) Let's call this function indirectly. This way we will only call it when the IC is meant to start.
(2) Let's remove all log Fatal statement. Instead, the function will return an error. If the error is not nil, we will exit in main.go
in main.go
in aws.go
this way we will be able to test checkAWSEntitlement.
Since we only make one external API call, we will be able to test it by wrapping it into an interface like this:
In unit tests, we will be able to create a fake implementation that will return the right output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of logic would you want to test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the
ifs in init()There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think
initas a whole needs a test as there's so little logic, justifs for the errors. You could wrap and test the logic around the token claims and unit test that?The question around the binary is interesting. Why would this code need a version check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a happy path when nothing returns an error and we a number places where the error needs to be checked. why would we not want to test it? 🤔
yeah, some additional refactoring of init can also help
this init code is always called, even if the version check command is invoked on the binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
supporting calling the registration from main is easy:
main.go
aws.go
However, the result is that (1) it allows using the same logger (glog) and (2) it will not affect the version check at all.
the requirement is to run that code as part of the Ingress Controller startup, so that the pod is registered. However, we don't have the requirement that it must be run from the
init()function. when implementing new features, in general we should not break the existing ones (in this case version check) and make sure that the new functionally fits nicely with the existing one (in this case, use the same logger for consistent format).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. but it works with a side effect - it will register a pod
Below is an example of inconsistent format:
fatal log in the init
fatal log in the main.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to offer some other considerations, for why @pleshakov's points merit consideration:
initmay be called beforemainbut there's no ordering guarantee for whichinitis called before another between packages. Not really the case here but it could be if more pakages are using init. This is implementation-dependent and the golang authors are free the change that implementation leading to silent breaksinitis magic and we should strive for simplicity rather than things happening automagically.initcan be used for setting up structures etc, since it's very early in the lifecycle of the process, using it to interact with state (the registration here) is beyond what would be good practice. IMO it would be better to explicitly initialize and control initialization frommainlog.Fatalininitis a recipe for surprise by developers down the line. I would not use that.my 2c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best solutions are always the most obvious that don't make you think and obviously this made a lot of people think! Thanks for all the great feedback, I will go with the suggested solution.