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

[cleanup][broker] Validate originalPrincipal earlier in ServerCnx #19270

Merged
merged 13 commits into from
Feb 6, 2023
Merged

[cleanup][broker] Validate originalPrincipal earlier in ServerCnx #19270

merged 13 commits into from
Feb 6, 2023

Conversation

michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Jan 18, 2023 •
edited
Loading

Motivation

The current ServerCnx validates the proxy role on certain protocol messages. This is unnecessary. Instead, we should verify that the authRole and the originalPrincipal are a valid combination before going to the Connected state.

Modifications

  • Replace invalidOriginalPrincipal with validateRoleAndOriginalPrincipal.
  • Remove calls to invalidOriginalPrincipal.
  • Add call to validateRoleAndOriginalPrincipal when transitioning from Connecting to Connected state.

Verifying this change

Added new test in ServerCnxTest to validate the authorization.

Documentation

  • doc-required

I updated the appropriate Javadocs, but we should also update the protocol spec to indicate valid combinations of the Connect command.

Matching PR in forked repository

PR in forked repository: michaeljmarshall#13

@michaeljmarshall michaeljmarshall added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use area/broker labels Jan 18, 2023
@michaeljmarshall michaeljmarshall added this to the 2.12.0 milestone Jan 18, 2023
@michaeljmarshall michaeljmarshall self-assigned this Jan 18, 2023
michaeljmarshall
michaeljmarshall commented Jan 18, 2023
View reviewed changes
Copy link
Member Author

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

There are some edge cases in the current solution that I think will require larger refactoring. Converting this to a draft for now.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Outdated Show resolved Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java Outdated Show resolved Hide resolved
@michaeljmarshall michaeljmarshall marked this pull request as draft January 18, 2023 19:30
@michaeljmarshall michaeljmarshall added doc-required Your PR changes impact docs and you will update later. and removed doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. labels Jan 18, 2023
@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Jan 18, 2023
@michaeljmarshall michaeljmarshall marked this pull request as ready for review February 5, 2023 06:54
@michaeljmarshall
Copy link
Member Author

Note that the one "breaking" change is to prevent connections where the originalPrincipal is set with an authRole that is not a proxy role. This is an invalid state that was allowed to persist before. That state was not a vulnerability because the isTopicOperationAllowed validates that both the originalPrincipal and the authRole have permission to perform an operation.

I removed this logic from the PR to make it easier to accept. It would be good to submit a follow up PR to propose making the broker's logic stricter.

@michaeljmarshall
Copy link
Member Author

@lhotari @nicoloboschi PTAL.

@codelipenghui codelipenghui mentioned this pull request Feb 6, 2023
Closed
5 tasks
@nodece
Copy link
Member

nodece commented Feb 6, 2023

/pulsarbot rerun-failure-checks

@lhotari lhotari merged commit fd3ce8b into apache:master Feb 6, 2023
@michaeljmarshall michaeljmarshall mentioned this pull request Feb 7, 2023
Merged
2 tasks
michaeljmarshall added a commit that referenced this pull request Feb 14, 2023
@michaeljmarshall
[cleanup][broker] Validate originalPrincipal earlier in ServerCnx (#1…
2847dd1 
…9270)

(cherry picked from commit fd3ce8b)
michaeljmarshall added a commit that referenced this pull request Feb 14, 2023
@michaeljmarshall
[cleanup][broker] Validate originalPrincipal earlier in ServerCnx (#1…
01bd986 
…9270)

(cherry picked from commit fd3ce8b)
(cherry picked from commit 2847dd1)
michaeljmarshall added a commit that referenced this pull request Feb 14, 2023
@michaeljmarshall
[cleanup][broker] Validate originalPrincipal earlier in ServerCnx (#1…
34a2c79 
…9270)

(cherry picked from commit fd3ce8b)
(cherry picked from commit 2847dd1)
(cherry picked from commit 01bd986)
@michaeljmarshall
Copy link
Member Author

I cherry-picked this change to the release branches because #19455 depends on it.

michaeljmarshall added a commit to datastax/pulsar that referenced this pull request Feb 14, 2023
@michaeljmarshall
[cleanup][broker] Validate originalPrincipal earlier in ServerCnx (ap…
b233d6b 
…ache#19270)

(cherry picked from commit fd3ce8b)
(cherry picked from commit 2847dd1)
(cherry picked from commit 01bd986)
michaeljmarshall added a commit that referenced this pull request Feb 14, 2023
@michaeljmarshall
[cleanup][broker] Validate originalPrincipal earlier in ServerCnx (#1…
9d36b0e 
…9270)

(cherry picked from commit fd3ce8b)
(cherry picked from commit 2847dd1)
(cherry picked from commit 01bd986)
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Feb 15, 2023
@nicoloboschi
Revert "[cleanup][broker] Validate originalPrincipal earlier in Serve…
8e9fe05 
…rCnx (apache#19270)"

This reverts commit b233d6b.
michaeljmarshall added a commit to datastax/pulsar that referenced this pull request Feb 15, 2023
@michaeljmarshall
[cleanup][broker] Validate originalPrincipal earlier in ServerCnx (ap…
5b0a564 
…ache#19270)

(cherry picked from commit fd3ce8b)
(cherry picked from commit 2847dd1)
(cherry picked from commit 01bd986)
@momo-jun
Copy link
Contributor

Hi @michaeljmarshall did you have any plans to update the protocol spec for this cleanup? Just want to make sure it can catch up with the release schedule.

@michaeljmarshall michaeljmarshall deleted the cleanup-proxy-role-validation branch March 6, 2023 22:20
@michaeljmarshall michaeljmarshall 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 Apr 20, 2023
@michaeljmarshall
Copy link
Member Author

The docs were updated here: apache/pulsar-site#408.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoloboschi nicoloboschi nicoloboschi left review comments

@lhotari lhotari lhotari approved these changes

@nodece nodece nodece approved these changes

@freeznet freeznet Awaiting requested review from freeznet

@zymap zymap Awaiting requested review from zymap

Assignees

@michaeljmarshall michaeljmarshall

Labels
area/broker cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. doc-complete Your PR changes impact docs and the related docs have been already added. ready-to-test release/2.8.5 release/2.9.5 release/2.10.4 release/2.11.1 type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@michaeljmarshall @nodece @momo-jun @lhotari @nicoloboschi