You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/10/29 20:56:15 UTC
[GitHub] [airflow] john-jac opened a new pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
john-jac opened a new pull request #19324:
URL: https://github.com/apache/airflow/pull/19324
When a Secrets Manager call fails due to the secret not being accessible to the IAM principle, rather than just not existing, it reports:
`botocore.exceptions.ClientError: An error occurred (AccessDeniedException) when calling the GetSecretValue operation: User: arn:aws:sts::xxx is not authorized to perform: secretsmanager:GetSecretValue on resource: yyy because no identity-based policy allows the secretsmanager:GetSecretValue action`
For example, if a user wants to limit an environment to secrets that are tagged with a specific resource. Today, this causes a failure with the Scheduler as an uncaught exception. This PR addresses that condition, plus adds an additional catch for other exceptions that we're not aware of yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-968316195
> The boto3 secrets manager library will fail with ResourceNotFoundException if there are no restrictions in the IAM role, i.e. it has full access to secrets manager. That isn't practical for most organizations, as they will have several entities accessing secrets manager and do not want to give full access to all secrets by all of them.
OK so what you're saying here is that it's `not practical for most organizations` to catch `ResourceNotFoundException` because in most organizations they'll get `AccessDeniedException` instead, because the cred exists but the instance does not have permission to access it.
But why is the airflow instance trying to retrieve the cred that it does not have access to in the first place? That seems like a misconfiguration issue. If the scheduler is trying to access, for example the value for `sql_alchemy_conn` from secrets backend (that's your scenario right?), and it is unable to do so, isn't the scheduler going to fail anyway?
Please help me understand. I will make time to be more responsive to this one going forward so we can make sure to get to resolution soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969428515
this is not airflow code. i can repro the exception of course, when i try to access a secret that i don't have access to.
but my point is, airflow does not try secrets backend for configs in general. it's only when you explicitly configure it to do so, for a specific config keys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-955146602
> Today, this causes a failure with the Scheduler as an uncaught exception
What do you mean with the scheduler? Perhaps most commonly one retrieves a connection from a celery worker. Are you running local executor? Or are you trying to use secrets backend for the metastore conn uri? I.e. was this a task that failed? Or some other scenario?
Can you help me understand what's the use case where you want to have the retrieval silently fail if access is denied? In this scenario do you store the secret in env vars?
In the "not found" scenario, the reason we suppress is secrets backend goes to the next secrets source (e.g. env var or metastore) when cred not found in the alt backend.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] john-jac commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
john-jac commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-968327100
> > The boto3 secrets manager library will fail with ResourceNotFoundException if there are no restrictions in the IAM role, i.e. it has full access to secrets manager. That isn't practical for most organizations, as they will have several entities accessing secrets manager and do not want to give full access to all secrets by all of them.
>
> OK so what you're saying here is that it's `not practical for most organizations` to catch `ResourceNotFoundException` because in most organizations they'll get `AccessDeniedException` instead, because the cred exists but the instance does not have permission to access it.
>
> But why is the airflow instance trying to retrieve the cred that it does not have access to in the first place? That seems like a misconfiguration issue. If the scheduler is trying to access, for example the value for `sql_alchemy_conn` from secrets backend (that's your scenario right?), and it is unable to do so, isn't the scheduler going to fail anyway?
>
> You have to jump through some real hoops to make secrets backend retrieve config keys from secrets backend. And this might not be such a good thing. But this reality means that if you have set up your cluster to retrieve a config key from secrets backend, I am not seeing how you'd also expect your cluster not to have access to that secret.
>
> Please help me understand. I will make time to be more responsive to this one going forward so we can make sure to get to resolution soon.
The fundamental way that a secrets backend works is to try all connections, variables, and configurations there first and, if it returns None, try the OS environment variables and finally the metadb.
In boto3, as a security feature, whenever you have any condition or resource constraint on Secrets Manager it ALWAYS returns Access Denied on a missing value, presumably so that one cannot phish for missing vs inaccessible values. This is currently uncaught, and as no user will ever have all of their configs and connections in Secrets Manager, leads to uncaught exceptions anywhere in Airflow trying to pull a config, which is pretty much everywhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-968316195
> The boto3 secrets manager library will fail with ResourceNotFoundException if there are no restrictions in the IAM role, i.e. it has full access to secrets manager. That isn't practical for most organizations, as they will have several entities accessing secrets manager and do not want to give full access to all secrets by all of them.
OK so what you're saying here is that it's `not practical for most organizations` to catch `ResourceNotFoundException` because in most organizations they'll get `AccessDeniedException` instead, because the cred exists but the instance does not have permission to access it.
But why is the airflow instance trying to retrieve the cred that it does not have access to in the first place? That seems like a misconfiguration issue. If the scheduler is trying to access, for example the value for `sql_alchemy_conn` from secrets backend (that's your scenario right?), and it is unable to do so, isn't the scheduler going to fail anyway?
You have to jump through some real hoops to make secrets backend retrieve config keys from secrets backend. And this might not be such a good thing. But this reality means that if you're trying to retrieve config from secrets backend, you should really expect your cluster to have access to that secret.
Please help me understand. I will make time to be more responsive to this one going forward so we can make sure to get to resolution soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969377230
> The fundamental way that a secrets backend works is to try all connections, variables, and configurations there first and, if it returns None, try the OS environment variables and finally the metadb.
Partially true. But, to make it retrieve config from secrets backend you have to jump through hoops.
E.g. you have to do something like this:
```
export AIRFLOW__CORE__SQL_ALCHEMY_CONN_SECRET=sql_alchemy_conn
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969377230
> The fundamental way that a secrets backend works is to try all connections, variables, and configurations there first and, if it returns None, try the OS environment variables and finally the metadb.
Partially true. But, to make it retrieve config from secrets backend you have to jump through hoops.
E.g. you have to do something like this:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] raphaelauv edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
raphaelauv edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-967341605
this is already implemented in the gcp secret_manager -> https://github.com/apache/airflow/blob/4f1e66d0227fba7a1378e895b9186711c03ead61/airflow/providers/google/cloud/_internal_client/secret_manager_client.py#L92
It make sence to have the same in thie aws secret_manager
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] raphaelauv edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
raphaelauv edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-967341605
this is already implemented in the gcp secret_manager -> https://github.com/apache/airflow/blob/4f1e66d0227fba7a1378e895b9186711c03ead61/airflow/providers/google/cloud/_internal_client/secret_manager_client.py#L92
It make sence to have the same in the aws secret_manager
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-955146602
> Today, this causes a failure with the Scheduler as an uncaught exception
What do you mean with the scheduler? Most commonly one retrieves a connection from a celery worker. Was this a task that failed? Or some other thing?
Can you help me understand what's the use case where you want to have the retrieval silently fail if access is denied? In this scenario do you store the secret in env vars?
In the "not found" scenario, the reason we suppress is secrets backend goes to the next secrets source (e.g. env var or metastore) when cred not found in the alt backend.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] john-jac commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
john-jac commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-956325118
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969377230
> The fundamental way that a secrets backend works is to try all connections, variables, and configurations there first and, if it returns None, try the OS environment variables and finally the metadb.
Partially true. But, to make it retrieve config from secrets backend you have to jump through hoops.
E.g. you have to do something like this:
```
export AIRFLOW__CORE__SQL_ALCHEMY_CONN_SECRET=sql_alchemy_conn
```
So if airflow is trying to retrieve a config from secrets backend, I think it's fair to say that it should exist and airflow should have access to it.
And I tested this with secrets manager yesterday, created an IAM account, gave it access to just one key (thanks for clarifying that scenario). And I observed that scheduler and webserver had no problem running. So I think config is a non-issue here.
But yes, if you ask for a conn that exists but you don't have access to it will throw that error. And yeah I think it's reasonable to add that catch. And now that you have removed `except Exception` I think we're in pretty uncontroversial territory. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-968316195
> The boto3 secrets manager library will fail with ResourceNotFoundException if there are no restrictions in the IAM role, i.e. it has full access to secrets manager. That isn't practical for most organizations, as they will have several entities accessing secrets manager and do not want to give full access to all secrets by all of them.
OK so what you're saying here is that it's `not practical for most organizations` to catch `ResourceNotFoundException` because in most organizations they'll get `AccessDeniedException` instead, because the cred exists but the instance does not have permission to access it.
But why is the airflow instance trying to retrieve the cred that it does not have access to in the first place? That seems like a misconfiguration issue. If the scheduler is trying to access, for example the value for `sql_alchemy_conn` from secrets backend (that's your scenario right?), and it is unable to do so, isn't the scheduler going to fail anyway?
You have to jump through some real hoops to make secrets backend retrieve config keys from secrets backend. And this might not be such a good thing. But this reality means that if you have set up your cluster to retrieve a config key from secrets backend, I am not seeing how you'd also expect your cluster not to have access to that secret.
Please help me understand. I will make time to be more responsive to this one going forward so we can make sure to get to resolution soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-968316195
> The boto3 secrets manager library will fail with ResourceNotFoundException if there are no restrictions in the IAM role, i.e. it has full access to secrets manager. That isn't practical for most organizations, as they will have several entities accessing secrets manager and do not want to give full access to all secrets by all of them.
OK so what you're saying here is that it's `not practical for most organizations` to catch `ResourceNotFoundException` because in most organizations they'll get `AccessDeniedException` instead. But why is the airflow instance trying to retrieve the cred that it does not have access to in the first place? That seems like a misconfiguration issue. If the scheduler is trying to access, for example the value for `sql_alchemy_conn` from secrets backend (that's your scenario right?), and it is unable to do so, isn't the scheduler going to fail anyway?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] raphaelauv commented on a change in pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
raphaelauv commented on a change in pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#discussion_r748887411
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -253,3 +253,16 @@ def _get_secret(self, path_prefix, secret_id: str) -> Optional[str]:
secret_id,
)
return None
+ except self.client.exceptions.AccessDeniedException as e:
+ self.log.debug(
+ "An error occurred (AccessDeniedException) when calling the "
+ "get_secret_value operation: %s",
+ str(e),
+ )
+ return None
+ except Exception as e:
Review comment:
Since all the calls to the secret backends are inside a try catch , it's not necessary to add this last catch in your PR
see -> https://github.com/apache/airflow/blob/9519bf664b1c20ebec940fc1c3daa097eca351b4/airflow/models/variable.py#L270
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969377230
> The fundamental way that a secrets backend works is to try all connections, variables, and configurations there first and, if it returns None, try the OS environment variables and finally the metadb.
Partially true. But, to make it retrieve config from secrets backend you have to jump through hoops.
E.g. you have to do something like this:
```
export AIRFLOW__CORE__SQL_ALCHEMY_CONN_SECRET=sql_alchemy_conn
```
Accidentally clicked enter will be updating this for next few minutes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969402833
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969428515
this is not airflow code. i can repro the exception of course, when i try to access a secret that i don't have access to.
but my point is, airflow does not search secrets backend for configs in general. it's only when you explicitly configure it to do so, for a specific config keys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] raphaelauv commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
raphaelauv commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-967341605
this already implemented in the gcp secret_manager -> https://github.com/apache/airflow/blob/4f1e66d0227fba7a1378e895b9186711c03ead61/airflow/providers/google/cloud/_internal_client/secret_manager_client.py#L92
It make sence to have the same in thie aws secret_manager
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-967546660
> this is already implemented in the gcp secret_manager ->
>
> https://github.com/apache/airflow/blob/4f1e66d0227fba7a1378e895b9186711c03ead61/airflow/providers/google/cloud/_internal_client/secret_manager_client.py#L92
>
> It make sence to have the same in thie aws secret_manager
One difference is, there it does not catch _all_ exceptions but only certain ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] john-jac commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
john-jac commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969383922
> > The fundamental way that a secrets backend works is to try all connections, variables, and configurations there first and, if it returns None, try the OS environment variables and finally the metadb.
>
> Partially true. But, to make it retrieve config from secrets backend you have to jump through hoops.
>
> E.g. you have to do something like this:
>
> ```
> export AIRFLOW__CORE__SQL_ALCHEMY_CONN_SECRET=sql_alchemy_conn
> ```
Maybe so, but even without the above export, when secrets manager backend is set, and a condition or resource constraint is applied, Airflow will fail to start with multiple instances of `AccessDeniedException`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#discussion_r749731036
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -253,3 +253,10 @@ def _get_secret(self, path_prefix, secret_id: str) -> Optional[str]:
secret_id,
)
return None
+ except self.client.exceptions.AccessDeniedException as e:
+ self.log.debug(
+ "An error occurred (AccessDeniedException) when calling the "
+ "get_secret_value operation: %s",
+ str(e),
+ )
Review comment:
@john-jac not sure if you saw this comment? would you mind either implementing the suggestion or stating why not? i suspect it was auto-dismissed after a rebase or something.
thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969387824
> Maybe so, but even without the above export, when secrets manager backend is set, and a condition or resource constraint is applied, Airflow will fail to start with multiple instances of `AccessDeniedException`
odd. i was not able to repro that 🤷
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#discussion_r749588930
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -253,3 +253,10 @@ def _get_secret(self, path_prefix, secret_id: str) -> Optional[str]:
secret_id,
)
return None
+ except self.client.exceptions.AccessDeniedException as e:
+ self.log.debug(
+ "An error occurred (AccessDeniedException) when calling the "
+ "get_secret_value operation: %s",
+ str(e),
+ )
Review comment:
```suggestion
except self.client.exceptions.AccessDeniedException:
self.log.debug(
"An error occurred (AccessDeniedException) when calling the "
"get_secret_value operation",
exc_info=True,
)
```
`logging` has a built-in way to log an exception! 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#discussion_r749744305
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -253,3 +253,10 @@ def _get_secret(self, path_prefix, secret_id: str) -> Optional[str]:
secret_id,
)
return None
+ except self.client.exceptions.AccessDeniedException as e:
+ self.log.debug(
+ "An error occurred (AccessDeniedException) when calling the "
+ "get_secret_value operation: %s",
+ str(e),
+ )
Review comment:
oh looks like you resolved it i am not sure how i got on old commit never mind
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969428515
this is not airflow code. i can repro the exception of course, when i try to access a secret that i don't have access to.
but my point is, airflow does not try secrets backend for configs in general. it's only when you explicitly configure it to do so, for specific config keys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] john-jac commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
john-jac commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969417889
> > Maybe so, but even without the above export, when secrets manager backend is set, and a condition or resource constraint is applied, Airflow will fail to start with multiple instances of `AccessDeniedException`
>
> odd. i was not able to repro that 🤷
It might be dependent on how you're connecting to AWS services, however I was able to repro without Airflow just by assuming a role with boto3 such as this Python code:
```
my_role_arn = "<my role arn>"
# create an STS client object that represents a live connection to the
# STS service
sts_client = boto3.client('sts')
# Call the assume_role method of the STSConnection object and pass the role
# ARN and a role session name.
assumed_role_object=sts_client.assume_role(
RoleArn=my_role_arn,
RoleSessionName="AssumeRoleSession1"
)
# From the response that contains the assumed role, get the temporary
# credentials that can be used to make subsequent API calls
credentials=assumed_role_object['Credentials']
# Use the temporary credentials that AssumeRole returns to make a
# connection to Amazon SM
sm_client_assumed=boto3.client(
'secretsmanager',
aws_access_key_id=credentials['AccessKeyId'],
aws_secret_access_key=credentials['SecretAccessKey'],
aws_session_token=credentials['SessionToken'],
)
# Use the Amazon SM resource object that is now configured with the
# credentials to access your secret.
secret_id = "<some secret not there>"
secret_val_assumed = sm_client_assumed.get_secret_value(SecretId=secret_id)
```
Give `my_role_arn` access to secrets manager like:
```
{
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Action": [
"secretsmanager:GetResourcePolicy",
"secretsmanager:GetSecretValue",
"secretsmanager:DescribeSecret",
"secretsmanager:ListSecretVersionIds"
],
"Resource": "arn:aws:secretsmanager:us-west-2:012345678910:secret:some-prefix/*"
}
},
{
"Effect": "Allow",
"Action": "secretsmanager:ListSecrets",
"Resource": "*"
}
]
}
```
This will generate `AccessDeniedException`. Use `"Resource": "arn:aws:secretsmanager:us-west-2:012345678910:secret:*"` instead and you'll get `ResourceNotFoundException`.
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-968316195
> The boto3 secrets manager library will fail with ResourceNotFoundException if there are no restrictions in the IAM role, i.e. it has full access to secrets manager. That isn't practical for most organizations, as they will have several entities accessing secrets manager and do not want to give full access to all secrets by all of them.
OK so what you're saying here is that it's `not practical for most organizations` to catch `ResourceNotFoundException` because in most organizations they'll get `AccessDeniedException` instead, because the cred exists but the instance does not have permission to access it.
But why is the airflow instance trying to retrieve the cred that it does not have access to in the first place? That seems like a misconfiguration issue. If the scheduler is trying to access, for example the value for `sql_alchemy_conn` from secrets backend (that's your scenario right?), and it is unable to do so, isn't the scheduler going to fail anyway?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] raphaelauv commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
raphaelauv commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-968317482
Since all the calls to the secret backends are inside a try catch , it's not necessary to add the last catch in your PR @john-jac
see -> https://github.com/apache/airflow/blob/9519bf664b1c20ebec940fc1c3daa097eca351b4/airflow/models/variable.py#L270
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] john-jac commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
john-jac commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-967559744
> > this is already implemented in the gcp secret_manager ->
> > https://github.com/apache/airflow/blob/4f1e66d0227fba7a1378e895b9186711c03ead61/airflow/providers/google/cloud/_internal_client/secret_manager_client.py#L92
> >
> > It make sence to have the same in thie aws secret_manager
>
> One difference is, there it does not catch _all_ exceptions but only certain ones.
The intent was to future-proof the code, as regardless of the reason for the exception it should fall back. If required the last catch-all can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] john-jac commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
john-jac commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-956325118
> > Today, this causes a failure with the Scheduler as an uncaught exception
>
> What do you mean with the scheduler? Perhaps most commonly one retrieves a connection from a celery worker. Are you running local executor? Or are you trying to use secrets backend for the metastore conn uri? I.e. was this a task that failed? Or some other scenario?
When you specify a secrets backend, all requests for variables, configs, or connections go through that backend first. There are numerous errors with an uncaught exception from the secrets backend, including Scheduler, Worker, and Web Server
> Can you help me understand what's the use case where you want to have the retrieval silently fail if access is denied? In this scenario do you store the secret in env vars?
>
> In the "not found" scenario, the reason we suppress is secrets backend goes to the next secrets source (e.g. env var or metastore) when cred not found in the alt backend.
The boto3 secrets manager library will fail with ResourceNotFoundException if there are no restrictions in the IAM role, i.e. it has full access to secrets manager. That isn't practical for most organizations, as they will have several entities accessing secrets manager and do not want to give full access to all secrets by all of them. As such, organizations use IAM restrictions tied to a role ARN (i.e. role_arn in AWS connection type) to ensure that each entity only accesses what is allowed. For example, I may restrict an Airflow environment to only see secrets with the defined connections_prefix:,variables_prefix, and config_prefix. However, when you do so you do not get a ResourceNotFoundException on missing secrets, which as you indicate should go to the next secrets source, but rather you get AccessDeniedException, which is an uncaught exception and does not go to the next secrets source and fails to retrieve the desired value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] raphaelauv removed a comment on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
raphaelauv removed a comment on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-968317482
Since all the calls to the secret backends are inside a try catch , it's not necessary to add the last catch in your PR @john-jac
see -> https://github.com/apache/airflow/blob/9519bf664b1c20ebec940fc1c3daa097eca351b4/airflow/models/variable.py#L270
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] john-jac commented on a change in pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
john-jac commented on a change in pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#discussion_r749580901
##########
File path: airflow/providers/amazon/aws/secrets/secrets_manager.py
##########
@@ -253,3 +253,16 @@ def _get_secret(self, path_prefix, secret_id: str) -> Optional[str]:
secret_id,
)
return None
+ except self.client.exceptions.AccessDeniedException as e:
+ self.log.debug(
+ "An error occurred (AccessDeniedException) when calling the "
+ "get_secret_value operation: %s",
+ str(e),
+ )
+ return None
+ except Exception as e:
Review comment:
Removed. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] john-jac commented on pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
john-jac commented on pull request #19324:
URL: https://github.com/apache/airflow/pull/19324#issuecomment-969432414
> but my point is, airflow does not try secrets backend for configs in general. it's only when you explicitly configure it to do so, for a specific config keys.
Interesting...it definitely prevents the Scheduler from starting in some instances. Regardless, it's good to catch known exceptions. Thank you for all of the feedback!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish merged pull request #19324: Catch AccessDeniedException in AWS Secrets Manager Backend
Posted by GitBox <gi...@apache.org>.
dstandish merged pull request #19324:
URL: https://github.com/apache/airflow/pull/19324
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org