-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
RFD - Oracle join method #49480
RFD - Oracle join method #49480
Conversation
rfd/0192-oci-join.md
Outdated
compartments: ["my_compartment", "ocid1.compartment.oc1...<unique_ID>"] | ||
# Regions to allow instances to join from. Both full names ("us-phoenix-1") | ||
# and abbreviations ("phx") are allowed. If regions is empty or | ||
# a wildcard, instances can join from any region. | ||
regions: ["phx", "us-ashburn-1"] |
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.
This structure would mean that you have to allow joining from any region from any compartment.
Do we ever foresee a desire to further restrict this? Maybe something like:
allow:
- regions: [ phx ]
compartments: [ compartment1, compartment2 ]
- regions: [ syd ]
compartments: [ compartment3 ]
?
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 you're suggesting is what I intended, I'm not sure what the difference is between that and what I wrote (other than that I forgot to mention that tenancy is required).
I made a small mistake with instance principal permissions; they only have permission to list compartments by default, not get a compartment. If we stick will no extra permissions, all this means is that we can use compartment names in the Teleport provision token, only OCIDs. I think I prefer this solution since it's less work to set up, but if we do want to keep compartment names, configuring those permissions isn't a huge hassle either. |
I had a look at Vault's oci auth method https://developer.hashicorp.com/vault/docs/auth/oci It looks like what they do is:
The downside is this requires the vault server to have oci credentials and the following policy:
I think it's possible we could do a version of this, where instead of our auth server signing the authenticateClient request, the node would have to pre-sign THAT request and send it to auth, to send to oracle. The downside is this would unfortunately require every joining instance to have that AUTHENTICATION_INSPECT policy. The upside is we wouldn't be relying on getting the caller's tenant ID from the undocumented and unverified JWT. I'm not sure if this is viable for us but I wanted to share my findings |
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.
Hey team! I just submitted our review of the RFD.
Overall, the "Security" section sufficiently outlines potential security-related issues with the feature. As a recap:
Replay Attacks
Adding a custom header with the challenge that Teleport generates will ensure the legetimacy of the request as well as ensuring that any replys after the token is accepted will be rejected.
JWT Validation
Because Teleport is not able to validate the signature of the JWT, the application cannot be certaing that the claims have not been tempered. To this end, only use values/claims that you can independently verify. All other values should not be considered safe and used without additional validation and verification.
Server-Side Request Forgery (SSRF)
To enrol an Oracle Cloud resource, Teleport will have to perform an HTTP request to Oracle's API to retrieve a list of available resources. This request is in partial control to the request. Proper precautions need to be taked to ensure that SSRF attacks are properly mitigated. This entails:
- removing the ability for user-controlled URLs - Users should not have control over the destination of the request. This value should be statically defined in the application.
- strict input validation - Any values that are user-controlled and will be used to build the final request need to undergo strict input validation. Validation should be performed based on the expected parameter type (e.g. UUID) or a pre-defined allowlist of expected values. Any deviations from the expected values should result in an error
- HTTP redirects - To further protect the application from SSRF attacks, disable any unneeded HTTP redirects. This ensures that the target URL defined by Teleport is the final destination of the request
Let me known if you have any comments or questions.
Friendly ping @nklaassen @strideynet |
|
||
- Create a [dynamic group](https://docs.oracle.com/en-us/iaas/Content/Identity/Tasks/managingdynamicgroups.htm) | ||
that matches all the instances that will join Teleport. | ||
- Create the following policy: `Allow dynamic-group <dynamic-group-name> to inspect authentication in tenancy` |
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.
Have you been able to find much about what exactly this policy allows and if it seems acceptable to allow this for all nodes that need to join teleport? If it only allows you call the authenticateClient endpoint it seems okay to me, all it allows is for the node to get information about itself
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.
Friendly ping @strideynet |
rfd/0192-oci-join.md
Outdated
} | ||
|
||
message RegisterUsingOracleMethodResponse { | ||
string challenge = 1; |
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.
Nit: Perhaps a oneof is appropriate here in the request/responses?
678e592
to
fad4a4d
Compare
@viktor-doyensec-teleport could you please take another look? |
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.
Aside from the comment of the Oracle "Dynamic Groups", the RDF looks good to me!
|
||
- Create a [dynamic group](https://docs.oracle.com/en-us/iaas/Content/Identity/Tasks/managingdynamicgroups.htm) | ||
that matches all the instances that will join Teleport. | ||
- Create the following policy: `Allow dynamic-group <dynamic-group-name> to inspect authentication in tenancy` |
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.
Have you taken dynamic groups misconfigurations into consideration, such as lax rules that would allow unauthorized resources become group members and be allowed to join the cluster? These misconfigurations usually arises from membership conditions that depend on attributes that can be specified by low-privileged users (eg. tags).
If I understand the flow correctly, to join the cluster, an instance needs to satisfy the following pre-conditions:
- be in the expected tenant (satisfied by default)
- be in the expected compartment (this is optional)
- be in the dynamic group (this will be satisfied if the group is misconfigured)
In such a scenario, gaining access to the dynamic group would effectively allow the instance to join the cluster.
I don't consider this an issue on Teleport's side. However, I think an update to the RDF or future documentation to highlight the dangers of such misconfiguration will definitely be useful.
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.
In my mind, the membership of the dynamic group is not intended to be a criteria for which instances are allowed to join; it's just the only available vehicle for giving those instances the needed permissions. The provision token (expected tenancy/compartment/region) is the sole source of who's allowed to join. Ideally the matching rules would exactly match what's in the token; I've added that recommendation to the RFD and I'll include it in the future docs.
fad4a4d
to
ec6e968
Compare
This change adds an RFD for the Oracle Cloud join method.
Part of #41705.