Skip to content

http mixin: only send auth when strings not empty - #2634

Merged
sebix merged 2 commits into
certtools:developfrom
sebix:fix-http-auth
Sep 4, 2025
Merged

http mixin: only send auth when strings not empty#2634
sebix merged 2 commits into
certtools:developfrom
sebix:fix-http-auth

Conversation

@sebix

@sebix sebix commented Aug 12, 2025

Copy link
Copy Markdown
Member

Only set HTTP header 'Authorization' if username or password are set and are not both empty string as they are by default in the Manager

fixes #2590

@sebix sebix added this to the 3.4.1 Patch release milestone Aug 12, 2025
@sebix sebix added bug Indicates an unexpected problem or unintended behavior component: core labels Aug 12, 2025

@aaronkaplan aaronkaplan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not quite sure.... I think http auth need both username and pwd

Comment thread intelmq/lib/mixins/http.py Outdated
# auth settings
if self.http_username is not None:
# auth settings. username or password must exist (if only one, then this is likely an error, but we should try without auth) and not be an empty string
if self.http_username or self.http_password:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would it work only with a pwd? (no username?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure if I understand. What change do you request here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well a pure password in http auth without username is meaningless.. no? How do HTPT servers react?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Changed to and and added a warning if only one is given

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks :)

@aaronkaplan
aaronkaplan self-requested a review August 25, 2025 15:06
Only set HTTP header 'Authorization' if username or password are set and are not both empty string as they are by default in the Manager

fixes certtools#2590
@sebix

sebix commented Aug 25, 2025

Copy link
Copy Markdown
Member Author

Now also added a positive check for the Authorization header.

@sebix

sebix commented Aug 29, 2025

Copy link
Copy Markdown
Member Author

@aaronkaplan this PR is waiting for your approval

and add a testcase for the warning

@kamil-certat kamil-certat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is actually a workaround for Manager issue, just btw

@sebix

sebix commented Sep 4, 2025

Copy link
Copy Markdown
Member Author

This is actually a workaround for Manager issue, just btw

The manager bug triggers this, but it could also be a result of a bad manual edit of the config files

@sebix
sebix merged commit 9c71a9c into certtools:develop Sep 4, 2025
20 checks passed
@sebix
sebix deleted the fix-http-auth branch September 4, 2025 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Indicates an unexpected problem or unintended behavior component: core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTTP: If username and password are empty strings (default), Authorization is still used

3 participants