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

Improve some logging for OIDC logins #125

Closed
UKFr-DIZ opened this issue Oct 20, 2023 · 2 comments · Fixed by #126 or #130
Closed

Improve some logging for OIDC logins #125

UKFr-DIZ opened this issue Oct 20, 2023 · 2 comments · Fixed by #126 or #130
Assignees
Labels
enhancement New feature or request ready for release Issue is fixed and merged into develop, ready for next release
Milestone

Comments

@UKFr-DIZ
Copy link

Hi,

as discussed with @schwzr, it would be useful if you could improve two things for the logging.

  • When having a successful login with OIDC to the DSF FHIR Server, the jetty server still constantly write into the log that the X-ClientCert is essentially missing, which is the correct behaviour when logging in with OIDC.
app_1  | WARN jetty-server-23 - ForwardedSecureRequestCustomizer.getClientCert(62) | X-ClientCert header does not start with -----BEGIN CERTIFICATE----- or -----BEGIN%20CERTIFICATE-----%0A
  • When having a user account, which has no E-mail it will show ? for the following logmessage for example.
app_1  | INFO jetty-server-51 - AbstractAuthorizationRule.reasonSearchAllowed(345) | Search of Task authorized for identity 'uniklinik-freiburg.de/?'

Thanks

BR
Nam

@hhund hhund self-assigned this Oct 30, 2023
@hhund hhund added the enhancement New feature or request label Oct 30, 2023
@hhund hhund added this to the 1.3.1 milestone Oct 30, 2023
@hhund
Copy link
Member

hhund commented Oct 30, 2023

Hi @UKFr-DIZ,

regarding the "X-ClientCert header" message: The Apache httpd reverse proxy currently sends (null) as the value for the header if the user is not authenticated with a client certificate. Adding expr="-n %{SSL_CLIENT_S_DN}s" to Lines 39, 45 and 51 of the host-ssl.conf file, as in RequestHeader set X-ClientCert %{SSL_CLIENT_CERT}s "expr=-n %{SSL_CLIENT_CERT}", only sets the header if the value of %{SSL_CLIENT_CERT} is not empty, thus suppressing the warning. I will make the modification with the upcoming release.

Concerning the log messages with the ? e-mail address: We are using the e-mail address from client-certificates and access tokens as a unique identifier for human users. In a future release we will likely remove the option to authenticate human users with certificates or tokens that do not contain an e-mail address. In the meantime I will make a modification for the upcoming release that will display a "generated" e-mail address based on the iss and sub values from the access token.

@hhund hhund linked a pull request Oct 30, 2023 that will close this issue
@hhund hhund added the ready for release Issue is fixed and merged into develop, ready for next release label Oct 30, 2023
@UKFr-DIZ
Copy link
Author

Hi @hhund,

thank oyu for your reply !:)
From our site we would prefer if we could authenticate users using token without an email. Sysadmins from our site have in our local concept site special sysadmin accounts. These are accounts are personal but don't have an mail configured. I used the user federation in KC and sync the users as read only, so I can't change the email of the synced user. What is your reasoning that the accounts must have an e-mail account ?

@hhund hhund mentioned this issue Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready for release Issue is fixed and merged into develop, ready for next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants