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.

Sign up for GitHub

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

Jump to bottom

Escape PAM special characters properly #656

Merged
merged 6 commits into from
Dec 18, 2024
Merged

Escape PAM special characters properly #656

merged 6 commits into from
Dec 18, 2024

Conversation

d-w-moore
Copy link
Collaborator

@d-w-moore d-w-moore commented Nov 6, 2024 •
edited
Loading

Fix character escaping.

For servers in the 4.2 and 4.3 series, @ and & no longer need escaping. We shall still escape = and ; due to their use as part of kvp (key-value pair) strings in the iRODS protocol.

@d-w-moore d-w-moore changed the title 650.m Escape PAM special characters properly Nov 6, 2024
@d-w-moore d-w-moore marked this pull request as draft November 6, 2024 06:58
@d-w-moore d-w-moore self-assigned this Nov 6, 2024
trel
trel reviewed Nov 6, 2024
View reviewed changes
irods/connection.py Outdated Show resolved Hide resolved
@llp-rug
Copy link

llp-rug commented Nov 13, 2024

It looks like these changes address our issue #650 .

@trel
Copy link
Member

trel commented Nov 13, 2024

Very good - thank you @llp-rug.

@d-w-moore d-w-moore marked this pull request as ready for review November 14, 2024 17:20
alanking
alanking reviewed Nov 22, 2024
View reviewed changes
irods/connection.py Outdated Show resolved Hide resolved
irods/connection.py Outdated Show resolved Hide resolved
@alanking
Copy link
Contributor

Also please eyeball Codacy

alanking
alanking reviewed Nov 22, 2024
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Good. I think that this may be ready for squashing soon...

@alanking
Copy link
Contributor

Please rebase and squash and we'll have another look. That should take care of the commit you attempted to revert as it will not need to replay those changes on top of the work done in #663.

@d-w-moore d-w-moore force-pushed the 650.m branch 2 times, most recently from 41548dd to 911106f Compare December 16, 2024 15:39
@alanking
Copy link
Contributor

I think that covers all the review comments. I think we're ready for another squash.

@d-w-moore
Copy link
Collaborator Author

I think that covers all the review comments. I think we're ready for another squash.

Yes, almost ... but due to restoration of my lost work from yesterday, perhaps we want to take another look. See: c5bf14e

alanking
alanking reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

New changes look alright to me. Let's wait for one more set of eyeballs before we begin the squashing

korydraughn
korydraughn reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Looks good.

Squash to taste if the tests are passing.

@d-w-moore
Copy link
Collaborator Author

Looks good.

Squash to taste if the tests are passing.

Tests pass.... squashing.

@alanking
Copy link
Contributor

alanking commented Dec 17, 2024 •
edited
Loading

#498 and #517 are closed issues in closed milestones - they should not be reopened. Please link different issues for those. #635 is also a closed issue which is being linked, but it's in the 3.0.0 milestone, so that's fine (IMO).

@trel
Copy link
Member

trel commented Dec 17, 2024

agreed on all counts.

@d-w-moore
Copy link
Collaborator Author

Ok, those issue numbers are replaced with new ones.

alanking
alanking approved these changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Excellent. # it.

@d-w-moore
[irods#650] new escaped character processing.
c6d775c 
This fixes the approach to escaping characters from the set: [@&;=]
that iRODS historically has had problems accommodating in PAM passwords
in the past.  Presently, for iRODS 4.2 and 4.3 we only need to consider
";" and "=" as problematic characters, due to the conflict with the
use of those characters in the KVP-formatted context parameter when the
AUTH_PLUG_REQ_AN api is used.
@d-w-moore
[irods#673,irods#665] modify test for internal irodsA generation
df7a0e2
@d-w-moore
[irods#583] add test for SSL connection timeout functionality
c566995
@d-w-moore
[irods#672] add test for PAM password expiration
63ada33
@d-w-moore
[irods#635] test against overwriting irodsA in native case
8dd3180 
That is, test the case of overwrite = False for the free function:
    irods.client_init.write_native_credentials_to_secrets_file
@d-w-moore
[irods#635] test against overwriting irodsA in pam case
7fb1853 
That is, test the case of overwrite = False for the free function:
    irods.client_init.write_pam_credentials_to_secrets_file
@alanking alanking merged commit 13dc9da into irods:main Dec 18, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trel trel trel left review comments

@alanking alanking alanking approved these changes

@korydraughn korydraughn korydraughn approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@d-w-moore @llp-rug @trel @alanking @korydraughn