You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2022/01/13 17:26:28 UTC
[GitHub] [guacamole-client] agreenbhm opened a new pull request #683: Added support for specifying LDAP UPN in properties file
agreenbhm opened a new pull request #683:
URL: https://github.com/apache/guacamole-client/pull/683
Added support for specifying LDAP UPN in properties file. Allows for any user in LDAP (with the corresponding UPN) to authenticate. Removes requirement of users being within same OU for large LDAP deployments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] agreenbhm commented on pull request #683: Added support for specifying LDAP UPN in properties file
Posted by GitBox <gi...@apache.org>.
agreenbhm commented on pull request #683:
URL: https://github.com/apache/guacamole-client/pull/683#issuecomment-1012419890
> @agreenbhm : A couple of notes:
>
> 1. You need to have a Jira issue opened for a pull request, and the PR and commits need to be tagged with the Jira issue.
> 2. There's already a Jira issue for this: https://issues.apache.org/jira/browse/GUACAMOLE-536
> 3. There's already work being done on this front: [GUACAMOLE-536: Implement additional bind types for LDAP #507](https://github.com/apache/guacamole-client/pull/507)
Thanks for the info. Looks like that PR is nearly 2 years old. @necouchman: Given that you are the author of that PR, is there anything more that is needed to be done to get that merged? It looks like the changes I made are pretty similar to the ones you did so I'm not sure why your PR wouldn't be merged yet since it seems functional.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #683: Added support for specifying LDAP UPN in properties file
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #683:
URL: https://github.com/apache/guacamole-client/pull/683#discussion_r789899794
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java
##########
@@ -120,17 +120,28 @@
* If required properties are missing, and thus the user DN cannot be
* determined.
*/
- private Dn getUserBindDN(LDAPConfiguration config, String username)
- throws GuacamoleException {
+ private Dn getUserBindDN(LDAPConfiguration config, String username, String password) throws GuacamoleException {
// If a search DN is provided, search the LDAP directory for the DN
// corresponding to the given username
- String searchBindLogon = config.getSearchBindDN();
+
+ String searchBindLogon;
+ String searchBindPassword;
+
+ if(confService.getUPNDomain() != "" && confService.getUPNDomain() != null){
+ searchBindLogon = username + "@" + confService.getUPNDomain();
+ searchBindPassword = password;
Review comment:
I don't think this should be done. The search bind DN is a very specific thing - it's the service account that Guacamole uses to resolve the DN of a user. It shouldn't be replaced with the user's own account.
If the idea here is to allow logins via UPN and avoid the search bind DN entirely, then the solution would be to allow user authentication to proceed without a user search by directly binding with the provided UPN - essentially an alternative to the direct user mapping already supported.
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java
##########
@@ -120,17 +120,28 @@
* If required properties are missing, and thus the user DN cannot be
* determined.
*/
- private Dn getUserBindDN(LDAPConfiguration config, String username)
- throws GuacamoleException {
+ private Dn getUserBindDN(LDAPConfiguration config, String username, String password) throws GuacamoleException {
// If a search DN is provided, search the LDAP directory for the DN
// corresponding to the given username
- String searchBindLogon = config.getSearchBindDN();
+
+ String searchBindLogon;
+ String searchBindPassword;
+
+ if(confService.getUPNDomain() != "" && confService.getUPNDomain() != null){
Review comment:
The test `confService.getUPNDomain() != ""` performs a by-reference comparison against the `String` object for `""`, not a comparison of the content of those strings. The proper test for whether a `String` is empty is `isEmpty()`, however the `null` check shown here would have to occur first for that call to not throw a `NullPointerException` in the event the string is indeed `null`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #683: Added support for specifying LDAP UPN in properties file
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #683:
URL: https://github.com/apache/guacamole-client/pull/683#issuecomment-1012355353
@agreenbhm : A couple of notes:
1. You need to have a Jira issue opened for a pull request, and the PR and commits need to be tagged with the Jira issue.
2. There's already a Jira issue for this: https://issues.apache.org/jira/browse/GUACAMOLE-536
3. There's already work being done on this front: #507
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper edited a comment on pull request #683: Added support for specifying LDAP UPN in properties file
Posted by GitBox <gi...@apache.org>.
mike-jumper edited a comment on pull request #683:
URL: https://github.com/apache/guacamole-client/pull/683#issuecomment-1018958629
I'm not sure whether these changes (in spirit) nor [GUACAMOLE-536](https://issues.apache.org/jira/browse/GUACAMOLE-536) are actually necessary:
* The LDAP support does allow for lookup of users based on their UPN. This would involve using `userPrincipalName` for the username attribute.
* Multiple LDAP servers are supported via the recently-introduced multi-server LDAP support. Users can be mapped to their relevant LDAP server based on username patterns, and then the search account can narrow that to a specific user once the relevant server is determined.
I know that Active Directory allows for binding with UPNs, and so adding a feature that would tell Guacamole to accept usernames matching that pattern and attempt to bind with those usernames directly would make configuration easier, but I'm not sure that's what these changes or [GUACAMOLE-536](https://issues.apache.org/jira/browse/GUACAMOLE-536) are intended to be.
**EDIT:** Sorry - that should be GUACAMOLE-536*. Copy-pasta of other things I also happen to have open.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #683: Added support for specifying LDAP UPN in properties file
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #683:
URL: https://github.com/apache/guacamole-client/pull/683#issuecomment-1018958629
I'm not sure whether these changes (in spirit) nor [GUACAMOLE-1511](https://issues.apache.org/jira/browse/GUACAMOLE-1511) are actually necessary:
* The LDAP support does allow for lookup of users based on their UPN. This would involve using `userPrincipalName` for the username attribute.
* Multiple LDAP servers are supported via the recently-introduced multi-server LDAP support. Users can be mapped to their relevant LDAP server based on username patterns, and then the search account can narrow that to a specific user once the relevant server is determined.
I know that Active Directory allows for binding with UPNs, and so adding a feature that would tell Guacamole to accept usernames matching that pattern and attempt to bind with those usernames directly would make configuration easier, but I'm not sure that's what these changes or [GUACAMOLE-1511](https://issues.apache.org/jira/browse/GUACAMOLE-1511) are intended to be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org