You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by semancik <gi...@git.apache.org> on 2017/03/10 16:12:45 UTC
[GitHub] httpclient pull request #66: CredSSP implementation, NTLM engine reworked
GitHub user semancik opened a pull request:
https://github.com/apache/httpclient/pull/66
CredSSP implementation, NTLM engine reworked
I have also significantly improved NTLM implementation in the HTTP client. It is not better aligned with Microsoft specifications and it supports more protocol options. I had to do it because CredSSP requires NTLM support for GSS API wrapping/unwrapping. So I have implemented it. The description, motivation and limitations are in the source code javadoc.
The code is tested with several different Windows servers.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Evolveum/httpclient credssp
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/httpclient/pull/66.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #66
----
commit 68b053d1b7379ca885dae91750d22bc25bf59539
Author: Radovan Semancik <ra...@evolveum.com>
Date: 2017-02-10T18:49:02Z
CredSSP implementation, NTLM engine reworked
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
@DaddyWri I am not feeling comfortable committing code that we are not able to maintain properly and the original contributor is unwilling to put any extra work into.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@neykov Ok, fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@semancik It may have escaped your notice, but all of us are volunteers here. I for one have a very busy life and am involved in many projects, including this one, that are wholly unrelated to my day job. I've been working on your patch for about 4 hours already and I'm nowhere near done yet. This is time I don't have other than what I have stolen from sleep.
I'm not pleased by the kind of attitude I'm seeing here; I can assure you that my request was not casual; I really *cannot* figure what you've actually done without reformatting the submission first. I tried to help you understand why your approach makes it hard for patch integrators, but instead you've been responding with anger and complaints.
If you think you've just been misunderstood, then by all means let us start over. If you want to work cooperatively to help us get your patch integrated, please show that you are willing to do that. It's not so simple as just accepting the merge, because the merge goes into trunk first (and thus your patch won't even align with the source files that are there and I had to address that first off).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/httpclient/pull/66
Any value seems to work, including the empty string.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
@semancik Try contributing to a larger ASF project such as CXF that impose _very_ strict style requirements and conformance with static code analysis rules and then come back. HC style check barely requires _anything_ beyond trivial whitespace discipline and certain basic import rules. Yet you have spent more time bitching about the style check than it would have taken it to reformat the patch
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/httpclient/pull/66
I'm also interested in seeing this merged so I offered my help to @semancik to get things moving here. As a start I'll try getting the formatting right and minimise white-space changes. Hope @semancik has some time to help on the testing side of things.
What else should be done to get this mergeable? Re-reading the thread and [@DaddyWri's comment](https://github.com/apache/httpclient/pull/66#issuecomment-286675869) looks like there are more changes needed to the internals - is it feasible to do them as part of this PR or refactoring should be carried out separately?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:
https://github.com/apache/httpclient/pull/66
@semancik @ok2c WDYT about putting links to msdn specification in Java docs?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
@DaddyWri I leave this patch at your mercy. You are the only person on the project both capable and willing to put some effort into NTLM related code, therefore your opinion matters most. Do not worry about branching. I'll take care of merging the code into all active branches. Should we decide to include this code it will likely go into the new 4.6.x branch
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@neykov Are you interested in helping to validate the final work? Do you have the Windows machines available to do this? I'm happy to finish writing the tests, but I think the code needs to be revalidated against a Windows instance somewhere -- for both CredSsp AND NTLM fetch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by semancik <gi...@git.apache.org>.
Github user semancik commented on the issue:
https://github.com/apache/httpclient/pull/66
My (not so humble) opinion is that the original contributor have put a lot of extra work already. The original contributor is just not willing to waste time. What is the real problem here? Is it that difficult to reformat the code when you already have the project formatting settings in your IDE? I do not have that. And I'm not willing to waste few hours looking up the setting and applying them just for this specific project. Even considering that you are not willing to put any "extra work" to the project by putting a reference to coding conventions to a project web site - or even a link to the source code. I'm sorry if this sounds harsh. I would just like to let you know how one of contributors submitting a non-trivial contributions feels about this. For the sake of future contributions to the project.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
Ok, NTLMEngineImpl is now back to being stateless.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
Ok, started. There's a branch: branches/pull-66, with the NTLMEngineImpl changes proposed (as modified by me). I'm going to start my day job now and will pick this up in the evening.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@ok2c I reviewed the entire diff and did not find any obvious problems. As I said, it's easy to miss something though. I'm reasonably happy if you want to commit it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@ok2c It also looks like you've migrated fully to git since I last worked on trunk on this project? My git-fu isn't good enough to set up a branch with these changes for local evaluation -- perhaps you can give me some hints as to the best way to do that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
Hi @semancik,
I wrote the original NTLM implementation and wanted to review your changes, but you changed the formatting. This is problematic both because it makes the diff hard to read and also because it violates the HttpComponents style checks and will thus not build without modifications.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@ok2c I had a long look at the NTLMEngineImpl changes. Basically:
- Trace level debug support, which I recommend we remove because it is quite unsecure if enabled;
- For the CipherGen embedded cipher class, no substantive changes were made at all, just method and member variable renaming and formatting; I recommend we don't include any of these, since they add noise and no value;
- NTLMEngineImpl is now stateful and contains the history of all messages, and that's basically necessary to allow signing and sealing. There must be considerable changes elsewhere to allow for this change in flow, which I have not looked at yet;
- Signing and sealing code, which constitutes the major addition to the engine itself. I recommend we take those changes provided unit tests are developed for them. There are a number of situations where signing and sealing support would allow future extensions to be worked in. I also think it would be good to consider taking the CredSSP implementation, once it is in form to do so.
As for timing -- since it appears that Mr. Semancik has no further interest in this work, it's likely to be a while before I can do it. Also, a trunk commit won't do much good for a backport to the 4.5.x branch since everything has moved, although with some effort a back patch might be developed.
I really wouldn't be concerned about proprietary legal problems; that ship sailed more than 15 years ago, and as Mr. Semancik points out, all of these specs are public now, and have been for more than a decade.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
@semancik do not change formatting of existing classes and you will be fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
@DaddyWri Karl, Do you think you could review changes to NTLMScheme and just ignore the rest? We could incorporate those changes if they make sense to you and leave the rest.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
I committed a null check and return of byte[0] to fix what you found. Thanks!!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
@DaddyWri Ignore HttpClient 5 entirely for now. Develop your code against 4.5 APIs either on GitHub or on a branch in SVN. I'll merge the changes to trunk once everything is ready.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by semancik <gi...@git.apache.org>.
Github user semancik commented on the issue:
https://github.com/apache/httpclient/pull/66
I'm willing to work cooperatively if you are willing to work cooperatively. Please update your website. Please put a link to the source code, coding conventions and IDE settings there. Then I can consider reformatting the code. But I will *not* reformat the code just to see another criticism how the formatting is wrong again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
The issue has been discussed with ASF legal ([see LEGAL-80](https://issues.apache.org/jira/browse/LEGAL-80?jql=project%20%3D%20LEGAL%20AND%20text%20~%20NTLM)). Given you are an existing ASF committer there should be no reason to not proceed. However, we still need to decide upon whether it can be added to 4.5 or we have to start 4.6 cycle in order to include this feature.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@ok2c Ah, my bad - it's still in svn, just moved to new places. Modifying the patch now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/httpclient/pull/66
@DaddyWri yes, I can try it out. I'm usually using eval images generated with the help of https://github.com/RoboticCheese/boxcutter-windows, but also have a Windows machine to play with.
Are there any existing live tests for fetching files or shall I just write something quick & dirty to GET the file?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
@semancik You were politely requested to reformat your code to reduce noise in the change set due to different formatting in order to make the changes easier to understand. This has nothing to do with the default style settings at all. If you cannot tweak your IDE to make it stop messing with formatting this is your problem. The "community over code" principle equally applies to @DaddyWri and the rest of us. Goodbye.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
The original change set has been committed to SVN 4.5.x, 4.6.x and trunk (5.0). Please review and close
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
Started working on the tests. Did the following:
(1) Changed CipherGen so that it accepts a Random and long time value as arguments, so that tests can be made repeatable;
(2) Added packet byte value checks to be sure that the Type1 and Type3 responses don't change without us knowing.
Still to do: test NTLM fetch against an IIS-protected file, any Windows version, This is necessary because the flags that are included now in the Type3Message are now the entire set of flags that come back in the Type2Message. This might be correct as far as I know because when I implemented it year ago there was no description of what the Type3Message flags field should contain. @semancik, when you said you "tried this against multiple Windows versions", did you try a straightforward NTLM file fetch to confirm that this still works?
If you haven't, and you would like to try this, you can check out the code at:
https://svn.apache.org/repos/asf/httpcomponents/httpclient/branches/pull-66
Please build and try it out, and let me know. I no longer have ready access to a Windows system with IIS running, which limits my ability to exercise this code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/66
Radovan,
NTLM used to be legal a legal minefield in the past. I still feel a bit uncomfortable just pulling in code related to Microsoft proprietary protocols without some due diligence. Was this change set produced under an employment contract and if so would your employer be willing to sign ASF [Corporate CLA](https://www.apache.org/licenses/#clas)?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@ok2c The changes to the NTLMScheme itself are relatively minor -- I think.
What I think we should do is reformat the submission and get a cleaner diff that way and then we can decide.
As for the new functionality, I agree we would do better to have someone on board longer term who understands it before committing it. Having said that, it does not appear to be terribly complicated, but it doesn't have any tests even and that's a concern. I'd volunteer to write some but I think we'd do far better to have the original author construct them, and I have been extremely pressed for time of late as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@neykov Looks like there's an error in the Type1Message builder; give me a moment to fix.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@neykov Oh, I didn't know about this! But yes, please write something quick and dirty to exercise an NTLM fetch against IIS with protected content, to be sure nothing broke.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by semancik <gi...@git.apache.org>.
Github user semancik commented on the issue:
https://github.com/apache/httpclient/pull/66
I have used public Microsoft documentation released as Microsoft Open Specifications:
https://msdn.microsoft.com/en-us/library/cc236621.aspx
https://msdn.microsoft.com/en-us/library/cc226764.aspx
AFAIK, there should be no issue as Microsoft quite specifically provides these documents to allow third-party implementations. But I'm not a lawyer and the details are not entirely clear to me.
When it comes to my employer, I'm in fact self-employed. I'm already Apache committer and I have signed the agreement. Does that count?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/httpclient/pull/66
Awesome, great work @semancik, @DaddyWri, @ok2c.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/httpclient/pull/66
CredSSP works as well. Found a small issue if I don't specify a domain in `NTCredentials`:
```
Caused by: java.lang.NullPointerException
at org.apache.http.impl.auth.CredSspScheme.encodeUnicode(CredSspScheme.java:569)
at org.apache.http.impl.auth.CredSspScheme.createAuthInfo(CredSspScheme.java:490)
at org.apache.http.impl.auth.CredSspScheme.authenticate(CredSspScheme.java:383)
at org.apache.http.impl.auth.HttpAuthenticator.doAuth(HttpAuthenticator.java:239)
at org.apache.http.impl.auth.HttpAuthenticator.generateAuthResponse(HttpAuthenticator.java:218)
at org.apache.http.impl.nio.client.MainClientExec.generateRequest(MainClientExec.java:224)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@neykov I would like to start with the NTLMEngineImpl modified as in the attachment above, plus the other changes I haven't yet looked at -- but this should all be against trunk/httpclient5, not the 4.x branches. Oleg and I will backport as needed when the patch is done.
The order of activity is roughly:
(1) Start with revised NTLMEngineImpl
(2) Add unit tests for new methods in NTLMEngineImpl
(3) Add in the other classes required for CredSSP; these currently don't integrate properly with the httpclient5 stuff so they need to be moved and updated accordingly
(4) Test everything against at least one Windows machine
(5) Let me have a look at the (saner) diff, so I can consider organizing the code a bit differently
(6) Retest against Windows if I wind up changing things
The httpclient5 stuff may not yet be in a usable state. If so, I hope @ok2c will let me know about that now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by semancik <gi...@git.apache.org>.
Github user semancik commented on the issue:
https://github.com/apache/httpclient/pull/66
Yes. That was my first thought. But IDE is imposing a formatting for a new code whether you want it or not. E.g. default IDE setting will indent empty lines which will make checkstyle very unhappy. So I have applied IDE settings that I though were the right ones. As I've already described several times. But it looks like you do not want to understand. Which is really a pity. I though that you might be interested in what a new contributor needs to go through. But maybe I was wrong. Maybe the "community over code" is not applied to all projects ...
Anyway. Contribution is there. I have done my best to prepare it. Then I was criticised for not following rules that were not stated. Even then I've tried to cooperate. But it looks like this does not lead anywhere. If you are not willing to even do this small improvement on your website to guide other contributors then I do not see any point in spending more time on this. So, you have the code. Take it or leave it. I already have my solution working. I just wanted to play nice, I went the extra mile and prepared my code for contribution as well as I could. Now I'm afraid that was all just wasted time. I'm sorry for that. Really sorry. Goodbye.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by semancik <gi...@git.apache.org>.
Github user semancik commented on the issue:
https://github.com/apache/httpclient/pull/66
I haven't reformatted the code on purpose. I was fighting with checkstyle, which got incredibly annoying during the development. I have wasted a lot of time with that. So I have applied IDE autoformatting with my Apache settings. As that fixed the checkstyle I haven't explored any further. As I haven't seen any explicit statement about code conventions on the site I have assumed that I'm doing the right thing. Honestly, I haven't thought for a second that there may be several incompatible code conventions in Apache Foundation. OK, now I know. One learns every day, obviously. Anyway, the code is here. If you feel like reformatting then please go ahead. But I won't do it. I do not think that it is fair to expect people to follow rules that the project does not even bother to state (or that cannot be found on the website even if I specifically look for it).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by semancik <gi...@git.apache.org>.
Github user semancik commented on the issue:
https://github.com/apache/httpclient/pull/66
Another note: if you remove trace devel then there will be no way how to debug the code. I think I have made that quite clear in the comments. The first thing to know about CredSSP is that it is usually TLS-in-TLS protocol and there is no way how to diagnose the issues using a packet sniffer or any similar tool. The devel trace is only real option if you want to have the implementation maintainable. I suggest you have a look at MS-CSSP and not just consider the NTLM part in isolation. In fact it is not easy to enable the devel trace by mistake. So what is the point? You are all the rage about how formatting is an obstacle to maintenance. And that can be easily fixed. And then you are suggesting to remove one of the things that make maintainability possible? I'm sorry, but I do not really get it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
The branch, by the way, now has everything in it except the NTLMScheme changes, which essentially did nothing more than deprecate the standard NTLMScheme, which was not a reasonable way of doing things.
- I haven't written any new tests yet; patches welcome if anyone wants to tackle this
- It looks like Mr. Semancik wanted to fundamentally change the NTLMEngine API, which of course is
not going to be something that can be done for backwards compatibility reasons
- The NTLMEngine itself is now stateful, as discussed before; this approach doesn't impress me as being
a good idea, so I've made the methods that care about the previous NTLMMessages in the state be
explicit arguments to the Type3Message constructor (at least one such variant)
- My thought is that the CredSspScheme should do the job of keeping track of previous messages, since
it has to keep much state anyway, and then we can rip out the new statefulness of the NTLMEngine
since it doesn't really belong there
- Much of the packing of new packets, though, should be moved into NTLMEngineImpl
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@neykov Great! As soon as Oleg reviews it I'm ready to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
What would you recommend we do in that case? Default to empty string?
Karl
On Fri, Mar 17, 2017 at 4:50 PM, Svet <no...@github.com> wrote:
> CredSSP works as well. Found a small issue if I don't specify a domain in
> NTCredentials:
>
> Caused by: java.lang.NullPointerException
> at org.apache.http.impl.auth.CredSspScheme.encodeUnicode(CredSspScheme.java:569)
> at org.apache.http.impl.auth.CredSspScheme.createAuthInfo(CredSspScheme.java:490)
> at org.apache.http.impl.auth.CredSspScheme.authenticate(CredSspScheme.java:383)
> at org.apache.http.impl.auth.HttpAuthenticator.doAuth(HttpAuthenticator.java:239)
> at org.apache.http.impl.auth.HttpAuthenticator.generateAuthResponse(HttpAuthenticator.java:218)
> at org.apache.http.impl.nio.client.MainClientExec.generateRequest(MainClientExec.java:224)
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/httpclient/pull/66#issuecomment-287467740>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AE1w2I9x4pva-FzqdtVrFjnYEsduW9Mkks5rmvIwgaJpZM4MZkRC>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by semancik <gi...@git.apache.org>.
Github user semancik commented on the issue:
https://github.com/apache/httpclient/pull/66
Done. References added.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by semancik <gi...@git.apache.org>.
Github user semancik commented on the issue:
https://github.com/apache/httpclient/pull/66
The build worked fine for me. I have spend a lot of time fighting with checkstyle and make sure that it is satisfied. I have absolutely hated it, but I have done it. I have tried to find any description of coding conventions on project web site. But I have failed. Maybe because the it is incredibly hard to find almost anything on the site? Therefore I have used settings recommended by another Apache project. I have somehow assumed that Apache Java projects are used to the same coding conventions. Excuse me if I was wrong. However, I have spent a lot of time improving implementation of this nightmarish protocol. I really do not have more energy left reformatting the code once again. So, take it or leave it. I'm OK either way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
@ok2c I have a revised version of NTLMEngineImpl that has the new functionality in place. Passes tests and it should work without modification. Still need to add new tests for the new functionality, and figure out how to integrate the rest.
I made a number of changes to stay within the bounds of the API as it's defined, so we will obviously want a test of some kind before going forward with any of it. I expect that when the rest is looked at more changes will be needed; we'll see.
[66-NTLMEngine.diff.txt](https://github.com/apache/httpclient/files/845883/66-NTLMEngine.diff.txt)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
Need to add CredSsp unit tests, and ask somebody to try it out, but then I think the branch might be ready to commit back to 4.5.x.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by DaddyWri <gi...@git.apache.org>.
Github user DaddyWri commented on the issue:
https://github.com/apache/httpclient/pull/66
Hi @semancik, the Apache standard can be overridden on a group-by-group basis, but the global standards are as follows:
- 4 spaces for each indent, no tabs
- braces in this style:
if {
} else {
}
- single space after commas in functions
- no spaces after or before parens, e.g. "( 1, 2 )"
This project's checkstyle is pretty rigorous but does not enforce all of the Apache standards.
It is indeed a challenge to work with, but is pretty self-explanatory when it finds a problem.
More importantly, it's just simply impossible to read diffs that include global reformatting. I therefore discourage people from reformatting as a matter of course because it does make unnecessarily hard to evaluate submissions. I'm not sure why you felt it necessary to *change* the style that was in place, but I have plodded through the diff, as it is, and if others agree it's acceptable in the current form, I will defer to that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #66: CredSSP implementation, NTLM engine reworked
Posted by semancik <gi...@git.apache.org>.
Github user semancik commented on the issue:
https://github.com/apache/httpclient/pull/66
I'm OK with adapting to a coding conventions. I have done that for several projects. But I would like to know the conventions from the beginning. I have adapted my contributions to the checkstyle that you have set up - even if that was costing a lot of my time. I have adapted to conventions that I believed are applicable to your project (even if I have that formatting style personally). I have already done all of that. But, it turns out, I have used wrong conventions. And why is that? Because I could not find anything about your coding conventions on your website. I could not even find link to the source code on your website. Therefore look at that from my point of view: I have done all that I could do adapt to your project. And all I get for that is criticism. That isn't really a reason to be happy about my contribution. That is not a good motivation to put any extra work in that, is it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org