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

Remove krb5_get_max_time_skew portability hack #1875

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Remove krb5_get_max_time_skew portability hack #1875

wants to merge 4 commits into from
+1 −23

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Jul 31, 2024 •
edited
Loading

This function has been available in Kerberos libraries
for over ten years now.

@yadij yadij marked this pull request as draft July 31, 2024 19:05
@yadij
Drop krb5_get_max_time_skew portability hack
673ed92 
This function has been available in Kerberos libraries
for over ten years now.
@yadij yadij changed the title Add SQUID_CHECK_KRB5_GET_MAX_TIME_SKEW Remove krb5_get_max_time_skew portability hack Jul 31, 2024
@yadij yadij marked this pull request as ready for review July 31, 2024 19:26
@yadij yadij requested a review from rousskov July 31, 2024 19:26
@yadij yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Jul 31, 2024
rousskov
rousskov previously requested changes Jul 31, 2024
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

I am not going to judge whether krb5_get_max_time_skew() is available in enough environments we care about. Your call.

acinclude/krb5.m4 Outdated Show resolved Hide resolved
src/peer_proxy_negotiate_auth.cc Show resolved Hide resolved
src/peer_proxy_negotiate_auth.cc Show resolved Hide resolved
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jul 31, 2024
@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Jul 31, 2024
@yadij yadij requested a review from rousskov July 31, 2024 22:13
@rousskov rousskov dismissed their stale review July 31, 2024 22:20

Thank you for addressing my concerns.

rousskov
rousskov reviewed Jul 31, 2024
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

This function has been available in Kerberos libraries for over ten years now.

Can we say the same about any other functions/features checked in SQUID_CHECK_KRB5_FUNCS and such? It is not clear why we are "picking on" krb5_get_max_time_skew() and/or whether we should be removing similar Kerberos hacks as well.

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jul 31, 2024
@yadij
Copy link
Contributor Author

yadij commented Sep 17, 2024 •
edited
Loading

We can possibly say the same about other things. This one was adding complications to the other refactoring and was quick enough to verify. I do not want to complicate he refactor steps by combining multiple changes, even if they are somewhat related.

If there are no actual issues I would like to merge this in the next two days and continue with the larger work.

@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Sep 17, 2024
kinkie
kinkie approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@kinkie kinkie left a comment

Choose a reason for hiding this comment

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

let's ship it

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Sep 18, 2024
@kinkie
Copy link
Contributor

kinkie commented Sep 18, 2024

@yadij approving, but stopping short of shipping

squid-anubis pushed a commit that referenced this pull request Oct 26, 2024
@yadij @squid-anubis
Remove krb5_get_max_time_skew portability hack (#1875)
850a58d 
This function has been available in Kerberos libraries
for over ten years now.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 26, 2024
@squid-anubis squid-anubis added M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 26, 2024
squid-anubis pushed a commit that referenced this pull request Oct 29, 2024
@yadij @squid-anubis
Remove krb5_get_max_time_skew portability hack (#1875)
1c6a3d0 
This function has been available in Kerberos libraries
for over ten years now.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 29, 2024
squid-anubis pushed a commit that referenced this pull request Oct 31, 2024
@yadij @squid-anubis
Remove krb5_get_max_time_skew portability hack (#1875)
5ec78e9 
This function has been available in Kerberos libraries
for over ten years now.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 31, 2024
squid-anubis pushed a commit that referenced this pull request Nov 1, 2024
@yadij @squid-anubis
Remove krb5_get_max_time_skew portability hack (#1875)
3304be1 
This function has been available in Kerberos libraries
for over ten years now.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Nov 1, 2024
squid-anubis pushed a commit that referenced this pull request Nov 1, 2024
@yadij @squid-anubis
Remove krb5_get_max_time_skew portability hack (#1875)
d1134c9 
This function has been available in Kerberos libraries
for over ten years now.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Nov 1, 2024
squid-anubis pushed a commit that referenced this pull request Nov 3, 2024
@yadij @squid-anubis
Remove krb5_get_max_time_skew portability hack (#1875)
a3cec63 
This function has been available in Kerberos libraries
for over ten years now.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-failed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rousskov rousskov rousskov left review comments

@kinkie kinkie kinkie approved these changes

Assignees
No one assigned
Labels
M-failed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yadij @kinkie @rousskov @squid-anubis