awslabs / aws-encryption-sdk-specification

AWS Encryption SDK Specification
Other
30 stars 27 forks source link

ExcludeRegions/LimitRegions Client Supplier Null/Empty Region Specification #90

Closed acioc closed 4 years ago

acioc commented 4 years ago

We should have a clear specification around the behavior of the ExcludeRegions/ LimitRegions client suppliers and ensure this behavior is matched by all ESDK's. Although most/all ESDKs interpret the definitions of these client suppliers in similar ways, the behavior of handling null/empty regions could be different.

Specifically..

  1. How does the LimitRegions client supplier handle null/ no regions? If no region is passed in, should the client supplier GetClient fail? Proposal: Yes, it should fail, since there's no way to know if the default region (which will be host dependent) is part of the limited regions to get clients for.
  2. How does the ExcludeRegions client supplier handle null/ no regions? If no region is passed in, should the client supplier GetClient fail? Proposal: Yes, it should fail, since there's no way to know if the default region (which will be host dependent) is part of the regions to avoid.

I believe that, currently, some ESDKs just check region in regions or !(region in regions) but this behavior (and the handling of nulls) could be language specific.

seebees commented 4 years ago

The question is does this happen before or after the call to the underlying supplier. If it happens before then I assert that you should put in whatever value will be used to denote "Let the SDK work out the region"

mattsb42-aws commented 4 years ago

The way I interpret the two is:

Carrying on from that, as @seebees stated, one valid value for "region name value" is "whatever indicates to a client supplier that the desired region is unknown".

lavaleri commented 4 years ago

I feel like letting the user specify "idk" in the region list could be very easy to misunderstand if not documented properly. (and maybe even if documented properly...)

e.g.

For excludeRegionsClientSupplier, it would be very easy for a user to make the assumption that, if region A is in that list of excluded regions, that means I can assume that this client supplier will never pass me a client from region A. In order to make these assumptions, a user will have to make sure they configure that list with "idk".

For a limitRegionsClientSupplier, it would be very easy for a user to make the assumption that, because region A is not in that list of allowed regions, that means I can assume that this client supplier will never pass me a client from region A. In order to make any such assumption, they will need to make sure "idk" is not in that list.

If we want to allow this value in the exclude/limitRegionList, then I would want to make it explicitly clear to users that these suppliers aren't making promises about the clients they deliver, they are only making promises about whether they make requests on certain inputs.

acioc commented 4 years ago

I support @lavaleri 's view on this. It seems disingenuous to have an exclude supplier that says "exclude us-west-2" but since my host defaults to "us-west-2", and I call GetClient with no region, I can still get back a "us-west-2" client.

It seems a lot safer to prevent this and allow customers to implement their own client suppliers if they're okay with this behavior as opposed to allowing this open for interpretation.

seebees commented 4 years ago

There is a check before/check after problem, that I would leave to the implementation. It is not that I disagree that limit(A, B, C (r) => r === '' ? A : r ) is a difficult case. But the behavior for these two functions is backwards.

As the problem is described, limit has an intuitive behavior in the case of '' being excluded. Where as exclude only has this intuitive behavior in the case where I include ''.

Again, checking the region again after the client is created completely removes this problem.

acioc commented 4 years ago

Is there a good way to get the region after the client is created in all ESDK implementations? It sounds like the proposal here is to get a client, but then check the region before actually returning it (as opposed to not constructing it at all if it's not necessary).

mattsb42-aws commented 4 years ago

nit: We would want to check both before and after client generation.

Is there a good way to get the region after the client is created in all ESDK implementations?

I can't speak with certainty for all, but I think it would probably be a reasonable assumption that there is.

mattsb42-aws commented 4 years ago

Updating from offline discussions, I think we're in agreement on the following:

For brevity, I will be referring to both the allow regions and deny regions client suppliers collectively as "region filtering client suppliers".

acioc commented 4 years ago

Closing in favor of https://github.com/awslabs/aws-encryption-sdk-specification/issues/165