paws-r / paws

Paws, a package for Amazon Web Services in R
https://www.paws-r-sdk.com
Other
314 stars 37 forks source link

`sts` endpoint - respect `sts_regional_endpoints = regional` or `AWS_STS_REGIONAL_ENDPOINTS` #631

Closed daniepi closed 1 year ago

daniepi commented 1 year ago

Hi, Thanks for developing and mainting paws packages. It seems like AWS config sts_regional_endpoints = regional and/or envvar AWS_STS_REGIONAL_ENDPOINTS are not respected by paws. Returned endpoint is not of form

https://sts.{region}.amazonaws.com

but default

https://sts.amazonaws.com

This should be (hopefully) a simple fix. I can try to create PR, but would need some guidance on where exactly in the code base implement this. Thanks a lot!

DyfanJones commented 1 year ago

hi @daniepi thanks for finding this out :) Off the top of my head I believe here is a good starting point.

https://github.com/paws-r/paws/blob/main/paws.common/R/config.R https://github.com/paws-r/paws/blob/main/paws.common/R/client.R

I am happy to help with PR with you. First I have to address https://github.com/paws-r/paws/issues/632 so that paws.common isn't removed from the cran.

joakibo commented 1 year ago

@daniepi Based on the tips from @DyfanJones and the AWS docs for this at https://docs.aws.amazon.com/sdkref/latest/guide/feature-sts-regionalized-endpoints.html :

check_config_file_sts_regional_endpoint <- function(profile = "") {
  config_path <- get_config_file_path()
  if (is.null(config_path)) {
    return(NULL)
  }

  profile <- get_profile_name(profile)
  if (profile != "default") profile <- paste("profile", profile)

  config_values <- read_ini(config_path)

  if (is.null(config_values[[profile]])) {
    return(NULL)
  }

  sts_regional_endpoint <- config_values[[profile]]$sts_regional_endpoint

  return(sts_regional_endpoint)
}

# Get the AWS STS Regional Endpoint property
get_sts_regional_endpoint <- function(profile = "") {
  sts_regional_endpoint <- get_env("AWS_STS_REGIONAL_ENDPOINTS")
  if (sts_regional_endpoint != "") {
    return(sts_regional_endpoint)
  }

  sts_regional_endpoint <- check_config_file_sts_regional_endpoint(profile)

  return(sts_regional_endpoint)
}
joakibo commented 1 year ago

On the issue we're having where STS in the backend should use the regional endpoint, here is the call to assume role in the backend assuming role automatically with role_arn as set in config file: https://github.com/paws-r/paws/blob/a86d7fb3d4fd6ca97952bd8f05166ca3d20d72d3/paws.common/R/credential_providers.R#L314C10-L314C20

Region there is hardcoded, that seems not good and suggest also that changes. Anyway, it's this one here which needs to respect the regional endpoint as discussed above. If the above is implemented it seems like that should be covered.

joakibo commented 1 year ago

@DyfanJones I'm starting on implementing the fix for this. Can I push to branch here directly or do I need to fork? Tried to push but got access denied. Thanks!

DyfanJones commented 1 year ago

Hi @joakibo thanks for taking this on, really appreciate the help. Please fork. The easiest way is to fork and then branch on your fork. Once ready it should be fairly straight forward to create a PR. If you need any help with forking and branching please let me know :)

joakibo commented 1 year ago

I have created the PR, please have a look, need some input on strategy and availability of R functions.