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.

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

Reference: pubsub pulsar OIDC authentication #3655

Merged
merged 8 commits into from
Sep 18, 2023

Conversation

JoshVanL
Copy link
Contributor

@JoshVanL JoshVanL commented Aug 1, 2023

Document pulsar OIDC authentication options. implemented here.

@@ -80,6 +81,14 @@ spec:
| publicKey | N | A public key to be used for publisher and consumer encryption. Value can be one of two options: file path for a local PEM cert, or the cert data string value |
| privateKey | N | A private key to be used for consumer encryption. Value can be one of two options: file path for a local PEM cert, or the cert data string value |
| keys | N | A comma delimited string containing names of [Pulsar session keys](https://pulsar.apache.org/docs/3.0.x/security-encryption/#how-it-works-in-pulsar). Used in conjunction with `publicKey` for publisher encryption |
| authType | N | One of `"none"`, `"token"` or `"oidc"`. The type of authentication mechanism to use. Defaults to `"none"`, unless `token` is defined in which case it defaults to `"token"`. | "oidc" |
| token | N | `authType: token`. Token used for authentication. | [How to create pulsar token](https://pulsar.apache.org/docs/en/security-jwt/#generate-tokens)|
Copy link
Member

@msfussell msfussell Aug 1, 2023

Choose a reason for hiding this comment

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

Given the settings required for oidc I suggest that there is a separate example of how these are set in the metadata. For example this component https://docs.dapr.io/reference/components-reference/supported-pubsub/setup-azure-eventhubs/#azure-active-directory-aad-authentication shows how to set AAD authentication correctly.

And if there are any particular gochas outside of the the description and examples, add this into a paragraph or two. In other words be more specific to user on how to use the oidc settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will do 👍

@msfussell
Copy link
Member

@JoshVanL - Also there are conflicts on the v1.12 branch https://v1-12.docs.dapr.io/reference/components-reference/supported-pubsub/setup-pulsar/

Copy link
Collaborator

@hhunter-ms hhunter-ms left a comment

Choose a reason for hiding this comment

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

quick review

@JoshVanL JoshVanL force-pushed the reference-pubsub-pulsar-oidc branch from 12db85e to a7e5f1f Compare August 3, 2023 14:07
@JoshVanL
Copy link
Contributor Author

JoshVanL commented Aug 3, 2023

@ItalyPaleAle please review

@@ -128,6 +134,42 @@ curl -X POST http://localhost:3500/v1.0/publish/myPulsar/myTopic?metadata.delive
}'
```

### OIDC Authentication

Since `v3.0`, [Pulsar supports OIDC authentication](https://pulsar.apache.org/docs/3.0.x/security-openid-connect/).
Copy link
Contributor

Choose a reason for hiding this comment

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

@JoshVanL would the changes we made support OAuth2 (without OIDC) too? https://pulsar.apache.org/docs/3.0.x/security-oauth2/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, we can add a certification test to see if it would work.

@@ -90,6 +89,13 @@ The above example uses secrets as plain strings. It is recommended to use a [sec
| processMode | N | Enable processing multiple messages at once. Default: `"async"` | `"async"`, `"sync"`|
| subscribeType | N | Pulsar supports four kinds of [subscription types](https://pulsar.apache.org/docs/3.0.x/concepts-messaging/#subscription-types). Default: `"shared"` | `"shared"`, `"exclusive"`, `"failover"`, `"key_shared"`|
| partitionKey | N | Sets the key of the message for routing policy. Default: `""` | |
| token | N | Token used for authentication. | [How to create Pulsar token](https://pulsar.apache.org/docs/en/security-jwt/#generate-tokens)|
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider creating a separate table for each authentication profile? (and include a mention that it can work without any). Example: https://docs.dapr.io/reference/components-reference/supported-state-stores/setup-sqlserver/#spec-metadata-fields

Copy link
Member

Choose a reason for hiding this comment

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

@JoshVanL - Can you address this comment from @ItalyPaleAle?

Copy link
Member

Choose a reason for hiding this comment

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

@JoshVanL - ping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msfussell all comments look to be resolved from my side I think.

@github-actions
Copy link

Stale PR, paging all reviewers

@github-actions github-actions bot added the stale label Aug 10, 2023
@msfussell msfussell removed the stale label Aug 10, 2023
@msfussell msfussell added this to the 1.12 milestone Aug 15, 2023
@github-actions
Copy link

Stale PR, paging all reviewers

@github-actions github-actions bot added the stale label Aug 22, 2023
@yaron2 yaron2 removed the stale label Aug 23, 2023
@github-actions
Copy link

Stale PR, paging all reviewers

@hhunter-ms
Copy link
Collaborator

@ItalyPaleAle could you review Josh's updates per your comments?

@github-actions
Copy link

Stale PR, paging all reviewers

@github-actions github-actions bot added the stale label Sep 11, 2023
@msfussell msfussell removed the stale label Sep 12, 2023
@github-actions
Copy link

Stale PR, paging all reviewers

@github-actions github-actions bot added the stale label Sep 18, 2023
@msfussell msfussell removed the stale label Sep 18, 2023
Copy link
Member

@msfussell msfussell left a comment

Choose a reason for hiding this comment

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

Few comments

JoshVanL and others added 3 commits September 18, 2023 17:15
…ubsub/setup-pulsar.md

Co-authored-by: Mark Fussell <[email protected]>
Signed-off-by: Josh van Leeuwen <[email protected]>
…ubsub/setup-pulsar.md

Co-authored-by: Mark Fussell <[email protected]>
Signed-off-by: Josh van Leeuwen <[email protected]>
…ubsub/setup-pulsar.md

Co-authored-by: Mark Fussell <[email protected]>
Signed-off-by: Josh van Leeuwen <[email protected]>
Copy link
Member

@msfussell msfussell left a comment

Choose a reason for hiding this comment

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

LGTM

@msfussell msfussell merged commit c2fc0a1 into dapr:v1.12 Sep 18, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants