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

Condensed Authentication bundle #1742

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JavaJoeS
Copy link

This security package contains functionality to allow for Client and/or Server Authentication using PKCS11 or PKCS12 keystores and JKS Truststores.

Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

  • Javadoc missing
  • Exception handling missing or should rethrow errors to caller
  • Externalizing Strings missing
  • Please remove all disabled code parts and temp files
  • Don't use static singletons, use proper service handling / injection / parameter passing

@JavaJoeS
Copy link
Author

@laeubi Pleasantly surprised, so few issues this go around. lol...

@laeubi
Copy link
Contributor

laeubi commented Feb 18, 2025

@laeubi Pleasantly surprised, so few issues this go around. lol...

rofl lmao

Beside that, this is just a quick review if things that need to be done before more in deep review... IMHO!

@JavaJoeS
Copy link
Author

@laeubi I could expect nothing less. Everyone wants to be secure, but no one wants security!

@merks
Copy link
Contributor

merks commented Feb 18, 2025

@sratz

FYI, when @HannesWell and I last talked with @JavaJoeS we strongly recommended that he get in contact with you because we don't know anyone who is qualified to review these low-level, security-related issues and I don't think we non-experts can move forward on technology where we do not understand fully the risks...

@JavaJoeS
Copy link
Author

@sratz Please contact me. Im in the process of doing updates as directed on this PR.

@sratz
Copy link
Member

sratz commented Feb 18, 2025

I am not a security expert so do not consider myself qualified to review this code. Besides the security aspects it's

  • way too many magic numbers and magic strings
  • repeated code
  • no clear public API that I can see at first glance
  • way too much commented / unused / ... code
  • simply too much for me to review at this time

I also fail to understand what concrete problem this is going to solve.

My point of contact with this kind of certificate handling is many because of

i.e., to ensure that the basic functionality of installing/updating software / talking to outside world in general works well also in weird corporate environments.
For that, my proposal would be a minimal solution of combining OS and JVM trust stores, not diving deeper into the security aspects.

I don't think this kind of complicated PCKS code belongs in the base platform as I believe it to be out of scope for an RCP platform.

Also, my personal experience is that the world is rather moving away from these kind of PKCS-based client/server authentication towards standards such as OAuth / OpenID Connect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants