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

OpenID Connect support #865

Merged
merged 7 commits into from
Jan 14, 2020
Merged

OpenID Connect support #865

merged 7 commits into from
Jan 14, 2020

Conversation

BrandonSchmitt
Copy link
Contributor

This pull request adds support for authentication providers implenting the OpenID Connect interface. It uses the Spring Security 5 features for OAuth2 respectively OpenID Connect.

The default configuration has not changed, thus the single user/tenant with basic auth is still the default method.

@BrandonSchmitt
Added OpenID Connect support
aa62275 
Utilized Spring Security's OAuth2 respectively OIDC support as another
possibility to manage users and their permissions.

Signed-off-by: Brandon Schmitt <[email protected]>
@BrandonSchmitt
Document OpenID Connect Support
f7a6a89 
Signed-off-by: Brandon Schmitt <[email protected]>
@hawkbit
Copy link

hawkbit bot commented Jul 15, 2019

Thanks for taking the time to contribute to hawkBit! We really appreciate this. Make yourself comfortable while I'm looking for a committer to help you with your contribution.
Please make sure you read the contribution guide and signed the Eclipse Contributor Agreement (ECA).

@schabdo schabdo added this to the 0.3.0M5 milestone Jul 15, 2019
@schabdo schabdo added the CQ label Jul 15, 2019
@schabdo
Copy link
Contributor

schabdo commented Jul 15, 2019

Thanks a lot for your contribution! OpenID connect support is highly appreciated in hawkBit. In order to fix the licence issue in the build just add a new licence-header file for your company here. The content provided first in OidcUserManagementAutoConfiguration.java was totally correct:

/**
 * Copyright (c) 2019 Kiwigrid GmbH and others.
 *
 * All rights reserved. This program and the accompanying materials
 * are made available under the terms of the Eclipse Public License v1.0
 * which accompanies this distribution, and is available at
 * http://www.eclipse.org/legal/epl-v10.html
 */

You deserve credit for your contribution in hawkBit for sure 😉

I’ll wrap my head around your PR and provide feedback. I assume testing this will take me quite a while

@BrandonSchmitt
Revert updated license notice and add Kiwigrid license file
4da932d 
This reverts commit 23d3624

Signed-off-by: Brandon Schmitt <[email protected]>
@BrandonSchmitt
Resolve SonarQube issues
be991e6 
- Explicitly import the needed specific classes
- Document public methods
- Add `static` to the constant `JwtAuthoritiesOidcUserService.INVALID_REQUEST`
- Remove superfluous runtime exception `OAuth2AuthenticationException`

Signed-off-by: Brandon Schmitt <[email protected]>
@BrandonSchmitt BrandonSchmitt mentioned this pull request Jul 15, 2019
Merged
3 tasks
@BrandonSchmitt
Copy link
Contributor Author

Thank you for your reply and the hint for the license.

I have resolved most SonarQube issues but the extra one:
SecurityManagedConfiguration.java#L612: Define a constant instead of duplicating this literal "/UI/logout" 3 times.
In my opinion, exactly following this advice and extracting this literal into a constant would result in inconsistencies in regard to the other literals like "/UI/login/**" in that function.
What is your opinion on this?

axdotl pushed a commit to kiwigrid/helm-charts that referenced this pull request Jul 15, 2019
@BrandonSchmitt @axdotl
[hawkbit-update-server] Added Docker image pull secrets and OpenID Co…
e09b9b3 
…nnect support (#132)

* Added Docker image pull secrets to the hawkbit update server

Signed-off-by: Brandon Schmitt <[email protected]>

* Removed empty line after imagePullSecrets if it is set.

`toYaml` currently and unintentionally creates a new line at the end of its output. This is going to be fixed in Helm 3 but not in Helm 2 since it would break existing charts (see helm/helm#3470).

Signed-off-by: Brandon Schmitt <[email protected]>

* Add support for OpenID Connect

OpenID Connect is not yet supported by the official hawBit project. However, a pull request has been opened concerning this feature. See eclipse-hawkbit/hawkbit#865.
This commit covers the changes introduced by that pull request.

The configuration uses the nested value `oidc`. The feature can be enabled by setting `oidc.enabled=true`.
In that case the following, additional values must be set:
oidc.clientId, oidc.clientSecret, oidc.issuerUri, oidc.authorizationUri, oidc.tokenUri, oidc.userInfoUri, oidc.jwkSetUri

The liveness and readiness probe has been changed accordingly to use a static resource which is always accessible without authentication.

Signed-off-by: Brandon Schmitt <[email protected]>

* Add oidc.enabled = false to the default values.yaml

Signed-off-by: Brandon Schmitt <[email protected]>

* Revert "Removed empty line after imagePullSecrets if it is set."

It increased complexity only for the sake of removing one empty line.

This reverts commit 374ca1a

Signed-off-by: Brandon Schmitt <[email protected]>

* Document OIDC and its status in hawkBit

Signed-off-by: Brandon Schmitt <[email protected]>

* Add starting space in comments

Signed-off-by: Brandon Schmitt <[email protected]>
@schabdo
Copy link
Contributor

schabdo commented Jul 16, 2019

In my opinion, exactly following this advice and extracting this literal into a constant would result in inconsistencies in regard to the other literals like "/UI/login/**" in that function

It's just static code analysis and general speaking a good advice. But in this case I totally agree. I'd prefer to see secured URL pattern right away instead of generic constant name. Furthermore changing the constant value later on might have unintended security side-effects. So long story short: No need to fix that

@hawkbit-bot

This comment has been minimized.

Sign in to view
schabdo pushed a commit that referenced this pull request Jul 25, 2019
CQ list for PR #865 populated
e80399b 
Signed-off-by: Dominic Schabel <[email protected]>
@schabdo schabdo modified the milestones: 0.3.0M5, 0.3.0M6 Jul 29, 2019
schabdo pushed a commit that referenced this pull request Aug 2, 2019
Filed CQs for PR #865
275c6a5 
Signed-off-by: Dominic Schabel <[email protected]>
@schabdo
Copy link
Contributor

schabdo commented Aug 2, 2019

Busy times in hawkBit ... but I made some progress and finished the paperwork for the Eclipse IP team

@schabdo
Copy link
Contributor

schabdo commented Sep 26, 2019

@BrandonSchmitt sorry, got distracted will work on it soon

@Informatic
Copy link

Don't want to nag everyone, but this seems like a great addition to hawkbit. Is this going to be merged anytime soon? :)

@schabdo schabdo removed the CQ label Jan 14, 2020
@schabdo
Copy link
Contributor

schabdo commented Jan 14, 2020

ECA valid and in place (checked manually). Thanks a lot @BrandonSchmitt for your patience!

@schabdo schabdo merged commit 1bcced9 into eclipse-hawkbit:master Jan 14, 2020
schabdo pushed a commit that referenced this pull request Jan 14, 2020
Sonar issues from PR #865 fixed
df21a63 
Signed-off-by: Dominic Schabel <[email protected]>
@axdotl
Copy link

axdotl commented Jan 15, 2020

Thanks for getting this merged.
Do you've information when one can expect an updated docker image?

@axdotl axdotl mentioned this pull request Jan 15, 2020
Merged
@schabdo
Copy link
Contributor

schabdo commented Jan 15, 2020

Building a new milestone will be my next task. This will include updated docker image as well. Expect to see it by end of next week

AmmarBikic pushed a commit to bosch-io/hawkbit that referenced this pull request Oct 2, 2020
CQ list for PR eclipse-hawkbit#865 populated
01d0216 
Signed-off-by: Dominic Schabel <[email protected]>
AmmarBikic pushed a commit to bosch-io/hawkbit that referenced this pull request Oct 2, 2020
Filed CQs for PR eclipse-hawkbit#865
d6c6a7b 
Signed-off-by: Dominic Schabel <[email protected]>
AmmarBikic pushed a commit to bosch-io/hawkbit that referenced this pull request Oct 2, 2020
@BrandonSchmitt
OpenID Connect support (eclipse-hawkbit#865)
fef2d6e 
* Added OpenID Connect support

Utilized Spring Security's OAuth2 respectively OIDC support as another
possibility to manage users and their permissions.

Signed-off-by: Brandon Schmitt <[email protected]>

* Document OpenID Connect Support

Signed-off-by: Brandon Schmitt <[email protected]>

* Updated license in OidcUserManagementAutoConfiguration.java

Signed-off-by: Brandon Schmitt <[email protected]>

* Revert updated license notice and add Kiwigrid license file

This reverts commit 23d3624

Signed-off-by: Brandon Schmitt <[email protected]>

* Resolve SonarQube issues

- Explicitly import the needed specific classes
- Document public methods
- Add `static` to the constant `JwtAuthoritiesOidcUserService.INVALID_REQUEST`
- Remove superfluous runtime exception `OAuth2AuthenticationException`

Signed-off-by: Brandon Schmitt <[email protected]>

* Add OidcUser support in SpringSecurityAuditorAware

Signed-off-by: Brandon Schmitt <[email protected]>

* Secure Management API using OpenID Connect, too.

Signed-off-by: Brandon Schmitt <[email protected]>
AmmarBikic pushed a commit to bosch-io/hawkbit that referenced this pull request Oct 2, 2020
Sonar issues from PR eclipse-hawkbit#865 fixed
c573d66 
Signed-off-by: Dominic Schabel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature request
Projects
None yet
Milestone
0.3.0M6
Development

Successfully merging this pull request may close these issues.
5 participants
@BrandonSchmitt @schabdo @hawkbit-bot @Informatic @axdotl