Fixing Vault AppRole authentication with CONN_URI

[nathadfield]

This PR adds some additional logic to the VaultHook to ensure that if a connection to Vault defined as a CONN_URI using AppRole authentication is used that the role_id is retrieved from connection.login.

closes: #18053

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

[potiuk]

Nice!

[github-actions]

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.

[potiuk]

Nice! Too fast approve though. Can you please update the docstring above? We have there the list of mappings login/passwords used for different authentication types and approle is now outdated.

We should take a look at aws_iam (but I guess there role_id should stay there as login is used as key_id there). I guess we should also raise a deprecation warning if role_id is used with "approle" authentication and tell users to move role to login in this case.

[nathadfield]

@potiuk No problem on the docstring.

Regarding the aws_iam, I did look at this briefly and it seemed like a CONN_URI for this authentication method would look the same and therefore also face the same problem.

Would you like me to add he deprecation warning too?

[nathadfield]

@potiuk If I add the deprecation warning should I also change the tests that now flag the warnings?

[potiuk]

Regarding the aws_iam, I did look at this briefly and it seemed like a CONN_URI for this authentication method would look the same and therefore also face the same problem.

Not quite sure as I think aws_iam (at least it looks like from the docs) requires three things for authentication: key (login), secret (password) and role - see example here https://www.vaultproject.io/docs/auth/aws#code-example. So I believe role is fine for aws_iam and you should pass it by extra. But please double check,

@potiuk If I add the deprecation warning should I also change the tests that now flag the warnings

A test in test_vaulr.py would be nice indeed.

[nathadfield]

https://www.vaultproject.io/docs/auth/aws#code-example

Ah, ok. Quite right! Perhaps I should just separate out these two auth methods then.

[nathadfield]

@potiuk I'd appreciate your input again when you get a free moment. No rush though.

[potiuk]

Hmm. That got me thinking.I think this warning is not really needed here (and the above warning should be updated).

As I understand the hook original design (and I have not designed it, just extended it) was that:

a) you can use connection to get all authentication information (login + password + extras)

b) you can override some of the information via parameters passed to the hook directly (and it is not 'deprecated' - this is perfectly valid way of overriding the "roles", key paths and other parameters if you choose to change them (so that for example you do not have to change the connection if in one task you decide to use different role for "aws_iam" for example.

c) however you can't override login/password because they are so "basic" authentication information that you REALLY want separate connection if you change one of those is different.

Now - this change deprecates overriding of role for "aws_iam" role and deprecates overriding "role" for approle via Hook parameters - which I think is not intended behaviour of the hook. You should still be able to override (without warning) the "aws_iam" role via Hook param, because it is not "basic" authentication information (secret and key are)

So I think this warning should not be generated here. Similarly - the warning above should be changed - we should only recommend using "login" to add "Approle" role.

I think there should still be a way to override the role in "approle" via Hook param (and for sure overrid

[potiuk]

Suggested change

	Please use either the connection login or extras.""",
	Please use connection login.""",

See my comment below

[nathadfield]

Ok, so, to clarify. No deprecation of role_id for aws_am and a modified warning for AppRole?

[potiuk]

Almost perfect :).

I think we not only want to warn when uses passes role_id for "approle" but we also want to deprecate this (pseudo-code):

dummy_login:password@host?auth_type="approle"&role_id="nn"

we should here raise a similar warning as below and ask users to change it to:

role:password@host?auth_type="approle"

[nathadfield]

@potiuk Yeah, fair enough. What about?

if auth_type == "approle":
    if role_id:
        warnings.warn(
            """The usage of role_id for AppRole authentication has been deprecated.
            Please use connection login.""",
            DeprecationWarning,
            stacklevel=2,
        )
    if self.connection.extra_dejson.get('role_id'):
        role_id = self.connection.extra_dejson.get('role_id')
        warnings.warn(
            """The usage of role_id in connection extra for AppRole authentication has been deprecated.
            Please use connection login.""",
            DeprecationWarning,
            stacklevel=2,
        )
    elif self.connection.login:
        role_id = self.connection.login

Happy to rephrase the messages as you suggest.

[nathadfield]

I think this provides different warnings for the current ways of submitting role_id without breaking anything.

[potiuk]

I'd say this (if-> elif):

if auth_type == "approle":
    if role_id:
        warnings.warn(
            """The usage of role_id for AppRole authentication has been deprecated.
            Please use connection login.""",
            DeprecationWarning,
            stacklevel=2,
        )
    elif self.connection.extra_dejson.get('role_id'):
        role_id = self.connection.extra_dejson.get('role_id')
        warnings.warn(
            """The usage of role_id in connection extra for AppRole authentication has been deprecated.
            Please use connection login.""",
            DeprecationWarning,
            stacklevel=2,
        )
    elif self.connection.login:
        role_id = self.connection.login

otherwise when you pass both role_id as parameter and login, the login one will be used, which is unexpected.

[nathadfield]

Good catch!