-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add Security bundles to support SSLContext values. These bundles consist of a headless version and a UI assist to update the Keystore and Truststore file locations #1716
base: master
Are you sure you want to change the base?
Conversation
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.
- PR description missing / insufficient
- License headers are wrong or outdated please update them
- TODO and code commented out has to be removed and or resolved
- System Outs has to be replace with proper logging
- exception handling has to be implemented
- Formating seems incositent, please apply Eclipse Formating profile
- JavaDoc is missing
- dont use enum for singeltons, either use directly static methods or proper instances
- "team" is not a suitable subfolder, please use the bundles subfolder
- Code has to be EPL 2.0 and contains contributions from other people might have to be properly vetted
- Uses service or plugin.xml to register adapter
- Observer is deprecated in java 9 replace with something else
- Avoid Serializable classes its is unclear why this is needed here as it imposes security risks
- Dont use Swing for UI components
- Message externalization missing in some places
- Handling exceptions by their message content is questionable
- Use ILog.get(...) directly instead of LogUtil
- Dont supress warnings, fix them
- Replace repetive static acces by local variable
- Dont init fields wih their default values
- Don't mix JUnit 4 and 5 Annotations, use JUnit 5 consitently
Wow, Sure would have been nice to have a seasoned Eclipse SR developer to work with me! Then prob none of these issues would be present. However, some seem to be subjective and petty, Again to thwart my attempt to give back to the community. |
While we welcome contributions they must meet certain criteria to be accepted and your PR does not pass the initial checklist here: Also usually all code has to be written by the person who creates the PR (or specifically handled, preferable in separate commits) so also from a legal perspective and this is also unclear here. So we are likely not able to accept this PR in the current form, but I gave you an additional checklist of concerns that are guaranteed to come up as soon as basic things are fulfilled. If then there are individual things that needs discussions this can take place in the PR comments during the review. |
Here you have the attention of Eclipse Platform Project Lead and PMC chair - I would call these "preliminary" issues before we even get into details. Esp when it's about "centralized" security component the expectations are that every tiny bit is properly commented and understood before a decision whether to continue with the review is to be taken. |
Eclipse Platform Project requires that new components/apis are accompanied by a PR/demo showing how they benefit/fix issue in Eclipse SDK (or another high profile Eclipse IDE project like EMF/CDT/LSP4E/....) . Without this further reviewing here will not happen. |
@akurtakov Thank you very much for jumping in. Im sorry for dumping my feelings.. Its been over a year that I have trying to commit and it just seems that Im getting pinged on some items that are subjective. I understand about the headers. Oversight on my part. Im not understanding the extra step of PR/demo etc. @scottlewis has updated ECF to fix some of the benefit of having a complete SSLContext. My bundles address the ability, that does not exist in eclipse for a custom Keystore/truststore. |
Take eclipse-platform/eclipse.platform.swt#1638 eclipse-platform/eclipse.platform.swt#1438 as an example - it provides a whole lot of data, various actions taken, reviews answered and it's still 6 months in the works . That's what it takes to contribute ! |
@akurtakov, I got that six months beat! I have a ton of disscusions in a lot of different groups in eclipse discussing my Bundles! Like a said Im over a year. I also recently submitted a document to explain more. What Im doing is NOT anything new. Its Java code to provide secure comms using an SSLContext. Nothing new to java world at all. Im failing to understand why the push back. If you search the internet there are tons of examples that show SSLContext implementations. |
For the record I still feel uncomfortable (actually a blocker for me!) discussing "security" code/concerns with unknown person as all I know about you is https://github.com/JavaJoeS which even lacks information like real name, company you work with and etc. As per https://www.eclipse.org/projects/handbook/ this PR would have to be vetted by IP team too as it's above 1000 LOC. |
Do you deliberately miss the part about different style of communication in svg issue and here? |
@akurtakov Im sorry for all these issues. Ive tried to address them. Im going to try and get to developer meeting this week and let folks get to know me and address some of these issues. |
In the SVG case one example is eclipse-platform/eclipse.platform.ui#2621 which allows developers to have the SWT and Platform UI change at the same time and see/test the benefit and uncover issues with the swt implementation. |
@akurtakov Here is a document I submitted already that can help. |
@JavaJoeS Two requests for you. If you fail to comply to both of them I'll close this PR as you seem to not pay full attention to requests from reviewers. These are not suggestions these are requirement!
|
@akurtakov Im on it. |
@akurtakov Updated github, will that suffice? |
@akurtakov Et al. See Screenshots.. |
@JavaJoeS You are still not pointing which existing Eclipse problem exactly will be improved by the UI you point to? It's the very last time I ask this question. |
Eclispe has many bundles/packages, each with their own SSLContext implementation. Some use apache imported package and some use home grown or JDK client. AGAIN, I have fielded many eclipse versions to companies with my solution baked in. Companies want their OWN Sonatype, their own NGINX, their own Apache and they want them LOCKED down. |
Eclipse update site url; |
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.
I second to close this PR without further discussion due to the low code quality mixed with accuses written in CAPS while not able to give a simple reproducer. |
Yes. A PR with 14kLOC is huge and just due to the amount of code only very difficult to review. I have made a few suggestions for smaller changes that would already be useful but should require much less code-changes in I think it would be simpler to start with those as independent changes, wouldn't it? |
Im currently working through the code removing any unsed and trying to shrink down, attempting to follow all modifications as directed herein. ANYONE wanna help, greatly appreciated. As said herein, lotta code! |
I suggest you 1. first describe the problem, 2. sketch the solution, 3. get approval for such enhancement and 3. find someone to volunteer as sponsoring reviewer, before you put a lot work into something that is likely to be rejected. Otherwise you will just waste your time. |
@jukzi Can you help? I have working copies of this code out that is used by hundreds of people. |
Sorry, i neither have the time for it nor do i see any value in this topic for eclipse IDE. Since the suggested PR only adds bundles without modifying existing bundles you could better work on a 3rd party plugin. However the best practice for security features is to NOT write any of them on your own but use existing, well tested solutions. |
Add bundles to provide custom Keystore and Truststore either headless or via UI.