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

[improve][authentication] Improve get the basic authentication config #16526

Merged
merged 1 commit into from
Aug 3, 2022
Merged

[improve][authentication] Improve get the basic authentication config #16526

merged 1 commit into from
Aug 3, 2022

Conversation

nodece
Copy link
Member

@nodece nodece commented Jul 11, 2022 •
edited
Loading

Signed-off-by: Zixuan Liu nodeces@gmail.com

Motivation

Right now the Pulsar only supports getting the basic authentication config from the system properties, I suggest reading it from the Pulsar config file, so like:

basicAuthConf=YOUR-BASE64
basicAuthConf=/my/basic.conf

Modifications

  • Add the basicAuthConf to the config file for use in reading the basic authentication config
  • Add Base64 support for basic authentication config

Documentation

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jul 11, 2022
gaoran10
gaoran10 reviewed Jul 12, 2022
View reviewed changes
Copy link
Contributor

@gaoran10 gaoran10 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great work!

gaoran10
gaoran10 approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM

@nodece nodece requested review from codelipenghui and tuteng July 12, 2022 08:47
@nodece nodece force-pushed the improve-basic-authen-config branch 3 times, most recently from 464ee32 to 8e2a324 Compare July 12, 2022 15:32
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 13, 2022
codelipenghui
codelipenghui reviewed Jul 13, 2022
View reviewed changes
conf/broker.conf
# basicAuthConf=/path/my/.htpasswd
# use Base64 to encode the contents of .htpasswd:
# basicAuthConf=YOUR-BASE64-DATA
basicAuthConf=
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
basicAuthConf=
basicAuthInBase64=

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want to add two config?

basicAuthInBase64=YOUR-BASE64-DATA
basicAuthInFile=/path/to/basic

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderBasic.java
Comment on lines +63 to +66
if (org.apache.commons.codec.binary.Base64.isBase64(data)) {
reader = new BufferedReader(new StringReader(new String(Base64.getDecoder().decode(data),
StandardCharsets.UTF_8)));
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use 2 different config/property names instead of checking isBase64, the isBase64(path) maybe return true?

Copy link
Member Author

@nodece nodece Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can add a prefix so like:

basicAuthConf=data:;base64,YOUR-BASE64
basicAuthConf=file:///my/basic.conf

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nodece Could you please update the PR as your description?

Copy link
Member Author

Choose a reason for hiding this comment

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

@codelipenghui This PR has been merged into master, this config still keep old style.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me open a new PR to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Link to #16935.

@codelipenghui codelipenghui added release/2.10.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/security area/authn labels Jul 13, 2022
@nodece nodece requested a review from codelipenghui July 14, 2022 09:05
@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@nodece
[improve][authentication] Improve get the basic authentication config
d7caf76 
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece force-pushed the improve-basic-authen-config branch from 8e2a324 to d7caf76 Compare August 3, 2022 07:34
@Technoboy- Technoboy- merged commit d32e1df into apache:master Aug 3, 2022
@Technoboy- Technoboy- modified the milestones: 2.12.0, 2.11.0 Aug 3, 2022
This was referenced Aug 3, 2022
Merged
Merged
nodece added a commit to nodece/pulsar that referenced this pull request Aug 4, 2022
@nodece
[improve][authentication] Improve get the basic authentication config (…
4efe8d4 
…apache#16526)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

(cherry picked from commit d32e1df)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece mentioned this pull request Aug 4, 2022
Merged
4 tasks
@nodece
Copy link
Member Author

nodece commented Aug 4, 2022

When cherry-picking this PR to branch-2.x, we also need to cherry-pick the #16935 to the same branch.

@nodece
Copy link
Member Author

nodece commented Aug 4, 2022

Move release/2.9.4 to #16944.

nodece added a commit to nodece/pulsar that referenced this pull request Aug 4, 2022
@nodece
[improve][authentication] Improve get the basic authentication config (…
15e0e5b 
…apache#16526)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

(cherry picked from commit d32e1df)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
This was referenced Aug 4, 2022
Merged
Merged
@nodece
Copy link
Member Author

nodece commented Aug 4, 2022

Move release/2.7.5 to #16947.

@nodece nodece deleted the improve-basic-authen-config branch August 4, 2022 10:51
@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Aug 5, 2022
BewareMyPower pushed a commit that referenced this pull request Aug 5, 2022
@nodece @BewareMyPower
[improve][authentication] Improve get the basic authentication config (…
acb4eba 
…#16526)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit d32e1df)
@BewareMyPower BewareMyPower added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Aug 5, 2022
codelipenghui pushed a commit that referenced this pull request Aug 8, 2022
@nodece @codelipenghui
[improve][authentication] Improve get the basic authentication config (…
aaedacf 
…#16526)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit d32e1df)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Aug 16, 2022
@nodece @nicoloboschi
[improve][authentication] Improve get the basic authentication config (…
713a9f4 
…apache#16526)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit d32e1df)
(cherry picked from commit aaedacf)
Technoboy- pushed a commit to merlimat/pulsar that referenced this pull request Aug 16, 2022
@nodece @Technoboy-
[improve][authentication] Improve get the basic authentication config (…
3829157 
…apache#16526)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaoran10 gaoran10 gaoran10 approved these changes

@tuteng tuteng Awaiting requested review from tuteng

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

Assignees

@nodece nodece

Labels
area/authn area/security cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.10 doc-complete Your PR changes impact docs and the related docs have been already added. release/2.8.4 release/2.10.2 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nodece @codelipenghui @gaoran10 @Technoboy- @BewareMyPower @Anonymitaet