Bugzilla – Full Text Bug Listing
| Summary: | Buffer changes for future SSL/TLS support | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Karel Gardas <karel.gardas> | ||||||||||||||
| Component: | cdo.net4j | Assignee: | Eike Stepper <stepper> | ||||||||||||||
| Status: | ASSIGNED --- | QA Contact: | Eike Stepper <stepper> | ||||||||||||||
| Severity: | enhancement | ||||||||||||||||
| Priority: | P3 | CC: | erwin, saulius.tvarijonas | ||||||||||||||
| Version: | 4.13 | ||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Solaris | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 333947 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Karel Gardas
Created attachment 184897 [details]
Net4j buffer changes for Net4j/TLS support
Created attachment 184933 [details]
Patch v2
The two new methods on Buffer look very similar to existing methods. I've tried to consolidate that for the startGetting case. While doing that I noticed that your if blocks differed slightly and I suspect that this was unintentional. Please correct me if I was wrong.
I also started to find potential for consolidation in the other "almost duplicate" method: write() / getBufferForWrite(). I have the feeling that your method name is not good. write() means "write to channel", i.e. read from buffer. I noticed this when I saw your call to byteBuffer.asReadOnlyBuffer() which implies that the result can not be written to. When we clarified these semantics and you confirmed that my first refactoring is working we can see how to refactor the remaining two methods so that they share the complicated IO logic. Further I'm renaming this bugzilla so that you can use it for the whole TLS feature. The Buffer changes alone are almost impossible to proof correct, hence I#d like to combine these changes with the coming changes for the overall feature. For the future please make sure that you create patches as *workspace-relative* (not project-relative). That makes it easier to apply them if I don't know what projects will be affected ;-) BTW. Buffer.java has not been changed after 3.0 SR1. That will make it trivial to port your changes to 4.0 ;-) (In reply to comment #2) > Created an attachment (id=184933) [details] > Patch v2 > > The two new methods on Buffer look very similar to existing methods. I've tried > to consolidate that for the startGetting case. While doing that I noticed that > your if blocks differed slightly and I suspect that this was unintentional. > Please correct me if I was wrong. I'm not sure, but what was exactly issue with my if block? I cannot find any change except that you have done while loop a more robust by also testing for hasRemaining in source buffer... Anyway your patch is working so I'm curious what exactly was my mistake for review...Thanks! (In reply to comment #3) > I also started to find potential for consolidation in the other "almost > duplicate" method: write() / getBufferForWrite(). I have the feeling that your > method name is not good. write() means "write to channel", i.e. read from > buffer. I noticed this when I saw your call to byteBuffer.asReadOnlyBuffer() > which implies that the result can not be written to. When we clarified these > semantics and you confirmed that my first refactoring is working we can see how > to refactor the remaining two methods so that they share the complicated IO > logic. getBufferForWrite means -- get real buffer representation which then is able to be written directly w/o any needed buffer preparation like saving channel id into it and such. When such buffer is available I'm able to encrypt it. Anyway if you find better name for it, I'm accepting what you change it to. :-) > For the future please make sure that you create patches as *workspace-relative* > (not project-relative). That makes it easier to apply them if I don't know what > projects will be affected ;-) Err, I'm simple minded c++/java/cal/haskell programmer using emacs mainly. I'm using eclipse when I just need to write something for it so I'm not so much eclipse power user. Anyway, I've finally found out what do you mean by this workspace-relative. Thanks for insisting on it. > getBufferForWrite means -- get real buffer representation which then is able to > be written directly w/o any needed buffer preparation like saving channel id > into it and such. When such buffer is available I'm able to encrypt it. Anyway > if you find better name for it, I'm accepting what you change it to. :-) That's what I expected first and then the name would be fine. But the ByteBuffer you're returning is created by asReadOnlyBuffer() and I doubt that you'll be able to write to it. In addition there seemd to be an unconditional clear() call on the original buffer. Possibly by a misplaced closing bracket of the preceeding if statement. > > For the future please make sure that you create patches as *workspace-relative* > > (not project-relative). That makes it easier to apply them if I don't know what > > projects will be affected ;-) > > Err, I'm simple minded c++/java/cal/haskell programmer using emacs mainly. I'm > using eclipse when I just need to write something for it so I'm not so much > eclipse power user. Anyway, I've finally found out what do you mean by this > workspace-relative. Thanks for insisting on it. Np. There are so many things in Eclipse that can make our lifes easier :P (In reply to comment #7) > > getBufferForWrite means -- get real buffer representation which then is able to > > be written directly w/o any needed buffer preparation like saving channel id > > into it and such. When such buffer is available I'm able to encrypt it. Anyway > > if you find better name for it, I'm accepting what you change it to. :-) > > That's what I expected first and then the name would be fine. But the > ByteBuffer you're returning is created by asReadOnlyBuffer() and I doubt that > you'll be able to write to it. In addition there seemd to be an unconditional > clear() call on the original buffer. Possibly by a misplaced closing bracket of > the preceeding if statement. Err, misunderstanding. By buffer for write I mean buffer which will be applied to socket channel write method, i.e. buffer from which we will just read. So I'm not going to write to it at all! Just read it. I see ;-) Eike, what about if I submit *current* stage of TLS support just to let you know better the usage of buffer functions? If you agree, please let me know exactly what to do for easy your work with it. FYI: I do have org.eclipse.net4j.tls project here which is just org.eclipse.net4j.tcp renamed, and where all the TCP strings/names were renamed to TLS. Also the only part I touched IIRC is TLSConnector.java (i.e. former TCPConnector.java). I'm able to do gtar -cjvf for you, but you surely do have eclipse preferred method for this. :-) Thanks! PS: the code is still to be clean up from a lot of debugging messages etc. so it looks horrible, but you will get an idea what's needed for this. I'd appreciate to have a look at your new code. A ZIP would be fine but the best way would be a patch that transforms TCPConnector into the new TLSConnector. Then I could focus on your changes and later we could copy the result into a new bundle. What do you think? Although this is more work for me now, I 100% agree with you that this is more clean approach so I did it. See attached bzip2ed patch file. WARNING: this is really prototype for now! Also for testing you will need to create some keystore... Created attachment 184953 [details]
Net4j TLS support 1st prototype
Okay, this is the right patch format, but please don#t zip it in the future ;-) I fear that there is another misunderstanding. In your patch you've renamed everything from tcp to tls (project, packages, classes). This way we certainly can not easily see the code changes. Is it possible that you "un-rename" everything so that I can apply the code changes to the original classes you've changed? Only this way I can get an overview of your changes as opposed to all the new state that is probably mostly a copy. This may also be helpful for the legal department that has to review the changes later on. I think this will be necessary anyway because we already agreed earlier that we probably don't want to create a big copy of the tcp plugin. When your actual changes are obvious we can decide whether to inherit the existing code or to integrate it directly somehow. (In reply to comment #15) > I fear that there is another misunderstanding. In your patch you've renamed > everything from tcp to tls (project, packages, classes). This way we certainly > can not easily see the code changes. Is it possible that you "un-rename" > everything so that I can apply the code changes to the original classes you've > changed? Only this way I can get an overview of your changes as opposed to all > the new state that is probably mostly a copy. This may also be helpful for the > legal department that has to review the changes later on. Hi Eike, I'm afraid if I rename everything back, then this will not run at all as this will be over-patched by Net4j provided TCP version which is already on version 3.0. Also I would like to point out that the project's choice of VCS which does not support renames is not my fault unfortunately. Anyway if you let me know how to solve this issue I described above I'll do as you like. In the meantime however I came with my own solution: please grab tcp project and rename everything in file/directory names from tcp to tls (and `TCP' to 'TLS'). And then attempt to apply the patch attached (v1b). This is a patch between straight TCP and my TLS implementation where I ignored renames so you will get exact data as you wanted in your last ask. Well, although this is raw patch and not something directly provided/supported by Eclipse. Please let me know your opinion. Thansk! Created attachment 185067 [details]
Net4j TLS support 1st prototype version b
(In reply to comment #18) > Created an attachment (id=185067) [details] > Net4j TLS support 1st prototype version b Ah, I also see there were some changes hapenning on tcp project since I kind of branch it. So they are also marked, but you will see them clearly IMHO. Please let me know what else shall I provide to you to easy the process of review. Thanks! Karel (In reply to comment #17) > I'm afraid if I rename everything back, then this will not run at all as this > will be over-patched by Net4j provided TCP version which is already on version > 3.0. Also I would like to point out that the project's choice of VCS which does > not support renames is not my fault unfortunately. Anyway if you let me know how > to solve this issue I described above I'll do as you like. In the meantime > however I came with my own solution: please grab tcp project and rename > everything in file/directory names from tcp to tls (and `TCP' to 'TLS'). And > then attempt to apply the patch attached (v1b). This is a patch between straight > TCP and my TLS implementation where I ignored renames so you will get exact data > as you wanted in your last ask. Well, although this is raw patch and not > something directly provided/supported by Eclipse. Please let me know your > opinion. I tried again to look at your code, but failed. Your latest patch is not a workspace patch. Please have a look at any of the hundreds of patches attached to other bugzillas to see how they look like. Your "patch" for example contains absolute paths from your particular file system. That can't be good. Further I don't understand what you mean by "as this will be over-patched by Net4j provided TCP version which is already on version 3.0". Am I right that the original tcp code/functionality is not needed while we work on your new tls functionality? Maybe I completely miss the point, but if I got you right you took the o.e.net4j.tcp bundle, copied it, renamed everything and applied tls specific changes. Copying and renaming was not a good idea because CVS indeed looses the connection ten (and please let's not discuss whose fault it is that we use CVS, we may switch to git in the near future). I still think the best way is that you 1) rename everything back to "tcp" 2) transfer the changes to the original tcp bundle (which is CVS connected and eases future compares/patches) 3) create a *workspace* relative patch from the changes in the original tcp bundle IMHO the only thing that could prevent us from doing that would be that your tls code requires the unmodified tcp bundle (which I doubt from all I know). Sorry that this turns out to be a hazzle. The best I can do is offer you to do it together via TeamViewer/Skype... Or, I could even do it alone. Then I'd need the newest snapshot (zip) of your tls bundle from you... Hi Eike, OK, I will do as you like, but I'll first finish rehandshake support and only a bit of cleanup of the debugging messages. I also see I still do have some issue with connection closure. Well, I hope I'll bring you what you request for Christmass. :-) I'll contact you next week in case of any troubles with this. Thanks for review attempt! (In reply to comment #22) > Hi Eike, > OK, I will do as you like, but I'll first finish rehandshake support and only a > bit of cleanup of the debugging messages. I also see I still do have some issue > with connection closure. Well, I hope I'll bring you what you request for > Christmass. :-) I'll contact you next week in case of any troubles with this. Wonderful ;-) > Thanks for review attempt! You're welcome! Created attachment 186301 [details]
Net4j TLS support 2nd prototype
So later than expected, but at least it's here finally. I've renamed back everything from TLS to TCP. As I claimed in the past this no longer (I mean plugin) work since the classes now mixes with original TCP plugin, but at least might be used for your review. Please let me know any suggestion w.r.t. code you have.
Thanks for review! Karel PS: if you like have it running, then please rename all *TCP*.java to *TLS*.java and this should run...
Hi Karel, I just finished a "mechanic" walk-through to make everything comply with our coding standards. It turned out that your only *important* modifications are to TCPConnector.java. If undone all the other changes (tcp->tls renamings mostly). This makes the patch smaller, but more important, we can run all of our CDO tests with the TLS implementation. I'll attach a new patch in a minute. When you create patches in the future make sure you don't include the .project in your patches.
When everything is in a state we consider final I'll take the task to create a separate bundle for it. For now my biggest problem is that the code depends on your file system:
java.io.FileNotFoundException: \export\home\karel\tmp\cdo-tls-keystore\keystore
How can I deal with it for first tests?
Later we need to decide how to pass the needed "secure things" into the connector. I have many more questions/comments. Is it possible that we have a screen sharing session? We use Skype and TeamViewer for this purpose.
BTW. Is this code:
KeyStore ksTrust = KeyStore.getInstance("JKS");
ks.load(new FileInputStream("/export/home/karel/tmp/cdo-tls-keystore/keystore"), passphrase);
okay? Shouldn't it be ksTrust.load... ?
Created attachment 186333 [details]
TLS patch v3
(In reply to comment #25) > Hi Karel, I just finished a "mechanic" walk-through to make everything comply > with our coding standards. It turned out that your only *important* > modifications are to TCPConnector.java. If undone all the other changes > (tcp->tls renamings mostly). Yes, you are right! > This makes the patch smaller, but more important, > we can run all of our CDO tests with the TLS implementation. I'll attach a new > patch in a minute. When you create patches in the future make sure you don't > include the .project in your patches. I don't know what you are talking about. :-) > When everything is in a state we consider final I'll take the task to create a > separate bundle for it. It would be also nice if you suggest how to add "magic" copyright sentence to the file for our changes. Certainly sponsor of those changes would like to have it there... > For now my biggest problem is that the code depends on > your file system: > > java.io.FileNotFoundException: > \export\home\karel\tmp\cdo-tls-keystore\keystore > > How can I deal with it for first tests? That's my testing keystore. What you will need is to create your own testing keystore together with testing trust store. The code is there exactly because of your note below. But certainly this might wait till we resolve our architectural questions from the beginning, i.e. how to integrate the stuff? Directly into TCPConnector, or inheriting all TCP stuff or cloning (like I did) etc. BTW: For tests you will need to use following properties: -Djavax.net.ssl.trustStore=/export/home/karel/tmp/cdo-tls-keystore/truststore -Dsun.security.ssl.allowUnsafeRenegotiation=true The first is for setting the truststore and the second is for allowing rehandshake which is disable in current JDK due to security issue with it, which is to be solved on Sun's side... > Later we need to decide how to pass the needed "secure things" into the > connector. That's exactly the reason things are hardwired now. I would certainly be happy to not have them in this form... > I have many more questions/comments. Is it possible that we have a > screen sharing session? We use Skype and TeamViewer for this purpose. > > BTW. Is this code: > > KeyStore ksTrust = KeyStore.getInstance("JKS"); > ks.load(new > FileInputStream("/export/home/karel/tmp/cdo-tls-keystore/keystore"), > passphrase); > > okay? Shouldn't it be ksTrust.load... ? Indeed, good catch, but this code is broken in its current form anyway. I wait for reorganization on fixing it, i.e. to have a framework how to pass secure things down to the connector... Please let me know if you do have any troubles creating your own working keystore/truststore and making everything run. Remember you need to rename everything back to TLS*... Thanks! Karel Hi Eike, sorry for not addressing your skype/teamviewer question. For skype I'm using dedicated M$ laptop, for development I'm using SunOS workstation hence teamviewer is not an option here...Well, I guess I'm right assuming this works only on M$. Thanks, Karel PS: I mean I'm happy to skype with you if this is enough for you. (In reply to comment #28) Is it possible that yousetup a CDO 4.0 development environment on your laptop while we're bringing this to an end? Note that we can not add this in 3.0 maintenance anyway. BTW. there's a linux version of Teamviewer. Not sure if that would help. (In reply to comment #27) > > When you create patches in the future make sure you don't > > include the .project in your patches. > > I don't know what you are talking about. :-) In your workspace you seem to have renamed the net4j.tcp project (and/or the contained plugin). That leads to a patch generated that I can not easily apply to the (unrenamed) project. I should have said "make sure that the .project file is not changed" ;-) > > When everything is in a state we consider final I'll take the task to create a > > separate bundle for it. > > It would be also nice if you suggest how to add "magic" copyright sentence to > the file for our changes. Certainly sponsor of those changes would like to have > it there... The copyright text is always the same, something like "... Eike Stepper and others.". This has several reasons. But you may and should change or add your name (if you want with a company) to the contributors list. Just have a look at the other sources. I'll try to make it run with your other suggestions... I just seem to realize that we have no own bugzilla for the SSLConnector feature. Karel, Can you please file a separate bugzilla? Then we continue from there... As you requested, here we go: https://bugs.eclipse.org/bugs/show_bug.cgi?id=333947 (In reply to comment #32) > As you requested, here we go: > https://bugs.eclipse.org/bugs/show_bug.cgi?id=333947 Thanks! I'll copy the patches over... Moving all open enhancement requests to 4.1 Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master. Moving all outstanding enhancements to 4.3 Moving all open enhancement requests to 4.4 Moving all open enhancement requests to 4.4 Moving all open bugzillas to 4.5. Moving all unaddressed bugzillas to 4.6. Moving all open bugs to 4.7 Moving all unresolved issues to version 4.8- Moving all unresolved issues to version 4.9 Moving to 4.13. |