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
PKI Implementation #873
PKI Implementation #873
Conversation
Dont forget to remove all of the print statements! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we can consider a review
- Please put appropriate license headers to all files
- Please squash your commits into a single one. Make sure the commit message describes efficiently the problem that this feature intends to solve
- Please describe in the PR a way to test it
- Please remove all unused code, commented code and all System.out
- Please use org.eclipse.core.net.pki package name
- Have you considered providing this support directly in existing org.eclipse.core.net/org.eclipse.ui.net bundles?
- If new bundle(s) is necessary, please split those 2 into 1 core and 1 ui bundles; we want security stuff to work even without UI (Eclipse Platform is used without its UI in various cases).
But the most important part is that you didn't yet make clear what problem you're trying to solve that cannot be already solved with proper JVM settings, and how one can verify the problem and the solution. Without it, this code will be forever ignored.
Export-Package: org.eclipse.core.pki, | ||
org.eclipse.pki.auth, | ||
org.eclipse.pki.dialog, | ||
org.eclipse.pki.exception, | ||
org.eclipse.pki.interfaces, | ||
org.eclipse.pki.jsse, | ||
org.eclipse.pki.pkcs, | ||
org.eclipse.pki.pkiselection, | ||
org.eclipse.pki.preferences, | ||
org.eclipse.pki.ui, | ||
org.eclipse.pki.util, | ||
org.eclipse.pki.wizard, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list must be shrinked to the bare minimal amount of packages that need to be exported. Ideally, nothing needs to be exported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, gotta figure out what really is needed if anything
Export-Package: org.eclipse.core.pki, | ||
org.eclipse.pki.auth, | ||
org.eclipse.pki.dialog, | ||
org.eclipse.pki.exception, | ||
org.eclipse.pki.interfaces, | ||
org.eclipse.pki.jsse, | ||
org.eclipse.pki.pkcs, | ||
org.eclipse.pki.pkiselection, | ||
org.eclipse.pki.preferences, | ||
org.eclipse.pki.ui, | ||
org.eclipse.pki.util, | ||
org.eclipse.pki.wizard, | ||
target.classes.org.eclipse.core.pki |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is clearly wrong. The target.classes
is not a package, it's a mistake coming from erroneous project configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The majority of files needs license header
org.eclipse.equinox.preferences;bundle-version="3.2.200", | ||
org.eclipse.osgi;bundle-version="3.4.0", | ||
org.eclipse.equinox.registry;bundle-version="3.4.0", | ||
org.eclipse.ui;bundle-version="3.203.100", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be another bundle with UI part, please do not mix it with "core" part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any help with this greatly appreciated
Automatic-Module-Name: org.eclipse.core.pki | ||
Bundle-ActivationPolicy: lazy | ||
Bundle-Activator: org.eclipse.pki.auth.AuthenticationPlugin | ||
Bundle-ClassPath: target/org.eclipse.core.pki-1.0.0.jar, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be replaced with
Bundle-ClassPath: .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I remove the target/**.jar, then plugin wont work.. Any suggestions? Im sure its me not being
so familiar with plugins...
@@ -0,0 +1,19 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed license header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I grab a header off file already in a bundle, copy and paste to all files needing header????
category="org.eclipse.ui.net.NetPreferences" | ||
class="org.eclipse.pki.preferences.PreferencePage" | ||
id="org.eclipse.preferences.pkcs" | ||
name="Java KeyStore Setup"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed i18n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does that go? on the name portion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to find another plugin.xml file so I could see where the i18n is supposed to be... None that I looked at had it??
/** | ||
* The activator class controls the plug-in life cycle | ||
*/ | ||
public class AuthenticationPlugin extends AbstractUIPlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one should be split to core
and ui
part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree ton of work needed here.
// The shared instance | ||
private static AuthenticationPlugin plugin; | ||
|
||
protected static final String DEFAULT_TRUST_STORE = "cacerts"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not clear why one would need protected
constants for Activator. I hope there is no plan to inherit from this type.
public static AuthenticationPlugin getDefault() { | ||
System.out.println("AuthenticationPlugin ---getDefault plugin"); | ||
if (plugin == null) { | ||
AuthenticationPlugin auth = new AuthenticationPlugin(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to support running without Platform for your plug-in? In this case I would consider another functionality entry point instead.
Moreover, activator should be an internal type that implements hooks for Bundle lifecycle. Ideally activator should not be present at all to avoid startup impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good question.. I think the answer should be YES. Support with or without GUI. The gotcha on this is
having all the required setup, more so for PKCS11 implementation..
In a nutshell, today's internet is tons of URLs which almost all are HTTPS single authentication sites. They are secured by X509 Certificates that (mostly) have been secured via public Certificate Authorities. They are single, basic authentication since we all use some browser or other that has the Certificate Authority already added in, or via a Java JDK where they are also embedded. HOWEVER, more and more companies, private entities, even some gubmints are creating internet sites with URLs that require two way authentication requiring YOU to have your own x509 Certificate in a Key Store signed by Their Certificate Authority. Eclipse DOES not contain this capability. The PKI Implementation I am trying to add, will provide this capability. |
Reduces compiler warnings in workspace and I-build reports.
Fixes the architecture link to the Eclipse wiki.
Where is the best place to write docs to describe usage/testing? I use my account from Amazon AWS to do lots of coding. UPDATE: I changed author/committer to same as my ECA.. Used git filter just like a pro JOE... lol |
I believe I have satisfied all the requests/modifications. |
PKI integration has been divided into two parts, ui and core for headless systems, as requested. I believe that I have completed all the requests per the review. Testing may be accomplished by creating a .pki file in your .m2 directory, or you can go to org.eclipse.core.pki.util and run the TemplateForPKIfile and it will create a pki.template file for you to fill in per your config.
My recommendation is use PKCS12 for simple testing. PKCS11 requires a vendor implementation such as OpenSC or the like.