Skip to content
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

PXP-10541 Client credentials rotation #1068

Merged
merged 10 commits into from
Jan 26, 2023
Merged

Conversation

paulineribeyre
Copy link
Contributor

@paulineribeyre paulineribeyre commented Jan 18, 2023

Jira Ticket: PXP-10541

New Features

  • Use the fence-create client-rotate command to receive a new set of credentials for a client without deleting the old credentials first. This allows for a rotation without downtime.

Deployment changes

  • Requires a Fence DB migration

@coveralls
Copy link

coveralls commented Jan 18, 2023

Pull Request Test Coverage Report for Build 13269

  • 67 of 75 (89.33%) changed or added relevant lines in 6 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.09%) to 74.48%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fence/config.py 1 2 50.0%
fence/resources/openid/idp_oauth2.py 0 1 0.0%
migrations/versions/a04a70296688_non_unique_client_name.py 15 16 93.75%
fence/scripting/fence_create.py 41 46 89.13%
Files with Coverage Reduction New Missed Lines %
fence/config.py 1 73.53%
fence/scripting/fence_create.py 1 60.18%
Totals Coverage Status
Change from base Build 13261: 0.09%
Covered Lines: 7121
Relevant Lines: 9561

💛 - Coveralls

)

print(f"Droppping 'unique client name' constraint: '{name_constraints[0]}'")
op.drop_constraint(name_constraints[0], "client")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure that it's entirely reuseable here, but there were some generic functions for adding/removing constraints in the old migration script:

def add_unique_constraint_if_not_exist(table_name, column_name, driver, metadata):
table = Table(table_name, metadata, autoload=True, autoload_with=driver.engine)
index_name = "{}_{}_key".format(table_name, column_name)
if column_name in table.c:
indexes = [index.name for index in table.indexes]
if index_name not in indexes:
with driver.session as session:
session.execute(
'ALTER TABLE "{}" ADD CONSTRAINT {} UNIQUE ({});'.format(
table_name, index_name, column_name
)
)
session.commit()
def drop_unique_constraint_if_exist(table_name, column_name, driver, metadata):
table = Table(table_name, metadata, autoload=True, autoload_with=driver.engine)
constraint_name = "{}_{}_key".format(table_name, column_name)
if column_name in table.c:
constraints = [
constaint.name for constaint in getattr(table.c, column_name).constraints
]
unique_index = None
for index in table.indexes:
if index.name == constraint_name:
unique_index = index
if constraint_name in constraints or unique_index:
with driver.session as session:
session.execute(
'ALTER TABLE "{}" DROP CONSTRAINT {};'.format(
table_name, constraint_name
)
)
session.commit()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed while writing this migration that the name of the index was different in my dev env than locally, so i had to figure out a generic way of finding the name.
These functions would have the same problem since they assume that the index name is "<table_name>_<column_name>_key". Maybe it's a cleaner way of querying the indexes though, if that's what you mean, i can change my code to do it the same way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion, if this didn't work out of the box then I think what you have is fine

@@ -34,6 +34,18 @@ def json_res(data):
return flask.Response(json.dumps(data), mimetype="application/json")


def generate_client_credentials(confidential):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docstring pls

@@ -276,6 +276,41 @@ def split_uris(uris):
logger.info(nothing_to_do_msg)


def rotate_client_action(DB, client_name, expires_in=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docstring pls

def rotate_client_action(DB, client_name, expires_in=None):
driver = SQLAlchemyDriver(DB)
with driver.session as s:
client = s.query(Client).filter(Client.name == client_name).first()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if we need to be more particular than .first() (vs getting the one with the latest expiration or something), but since everything is being copied every time to the new client, I think it's fine.

Copy link
Contributor

@Avantol13 Avantol13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants