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

Tests: add initial gssapi test using stub implementation #1687

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@iboukris
Contributor

iboukris commented Jul 18, 2017

The stub implementation is pre-loaded using LD_PRELOAD
and emulates common gssapi uses (only builds if curl is
initially built with gssapi support).

The initial test is currently disabled for debug builds
as LD_PRELOAD is not used then.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Jul 18, 2017

Hey, this aims to replace #761, simpler and not requiring the whole kerberos setup.
I'm checking the CI failures and I'll try to enable a build with gssapi as well, so the new test will run.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 20, 2017

Typo in commit message ("impelentation"), otherwise looks very good to me. The copyright dates are set to 2015, no idea if that matters.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Jul 20, 2017

Thanks @MarcelRaad! I'll fix those along with some other minor changes I have in mind.
I feel somewhat uncomfortable however about addind a new build to travis as it is now, though on the other hand it would be nice to build both with and without gssapi. Thoughts?

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 21, 2017

Instinctively, I would just change build type "normal" to include --with-gssapi and let the debug Travis builds and the autobuilds test the non-gssapi build, but I'm not the Travis expert.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Jul 21, 2017

Yea, I think I'll do that (remove the normal linux build) later on.
Meanwhile I've reworked the logic to make it easy to emulate krb5 and ntlm credential by parsing the creds environment variable for mechanism names.
I still have some troubles on server side with multiple round, as it appears 'swsbounce' won't work for more than one since req->partno always starts at 0, I trying to think how to get around this.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Jul 22, 2017

Ok, I gave up the usage of 'swsbounce' in favor of a special negotiate handler on the server which increments partno by one for each request with negotiate authorization header on the same test.
BTW, I forgot to clarify, the reason I didn't change "normal" to include --with-gssapi, is because that broke the "normal" build on osx, but we can keep both types and build only "gssapi" on linux.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Jul 26, 2017

btw, a slightly different approach, which would work with debug build, could be to add a configuration option to build the stub gss library as a static library and linked to it instead to real gss library so there no need for ld_preload (nor for real gss package).

@iboukris

This comment has been minimized.

Contributor

iboukris commented Aug 4, 2017

Update: I've rebased on master, and moved the memory-leak fix to another PR.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Aug 4, 2017

and moved the memory-leak fix to another PR

That was silly as now the corresponding test (2057) fails. I'll re-borrow that commit.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 4, 2017

I'll re-borrow that commit.

You don't need to, I'm just about to merge it.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 4, 2017

I merged the two PRs fixing the Travis failures now, so you just need to rebase on current master.

.travis.yml Outdated
@@ -26,6 +27,9 @@ matrix:
dist: trusty
env: T=normal
- os: linux
compiler: gcc
env: T=gssapi

This comment has been minimized.

@MarcelRaad

MarcelRaad Aug 5, 2017

Member

After the recent changes to this file, maybe just C=--with-gssapi could be added to the linux/normal build?

This comment has been minimized.

@iboukris

iboukris Aug 6, 2017

Contributor

Yeah, I'll try that, thanks!
I also want to try out the other approach suggested few comments above as a separate PR, so we can look at both approaches.

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@curl curl deleted a comment from coveralls Aug 5, 2017

@coveralls

This comment has been minimized.

coveralls commented Aug 21, 2017

Coverage Status

Coverage increased (+0.03%) to 73.267% when pulling 02c86ac on frenche:new_gss_tests into b939542 on curl:master.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Aug 21, 2017

(I gave up on the other approach as it has got complicated)
I've updated with some fixups, review appreciated.

@curl curl deleted a comment from coveralls Aug 21, 2017

@coveralls

This comment has been minimized.

coveralls commented Aug 21, 2017

Coverage Status

Coverage increased (+0.02%) to 73.248% when pulling a57ee53 on frenche:new_gss_tests into b939542 on curl:master.

@MarcelRaad

Looks good to me. I plan to try it out tomorrow.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 24, 2017

Sorry for the long delay!

With MSYS2 MSYS (x86_64-pc-msys) as well as Cygwin64 (x86_64-unknown-cygwin), configured with
--disable-debug --enable-curldebug --enable-warnings --enable-werror --enable-optimize --with-libmetalink --with-gssapi,
I get:

CC       libstubgss_la-stub_gssapi.lo
CCLD     libstubgss.la
libtool: warning: undefined symbols not allowed in [x86_64-pc-msys/x86_64-unknown-cygwin] shared libraries; building static only

and then:

test 2056...[HTTP Negotiate authentication (stub krb5)]

 2056: protocol FAILED:
--- log/check-expected  2017-08-24 17:41:24.201908300 +0200
+++ log/check-generated 2017-08-24 17:41:24.200905600 +0200
@@ -2,8 +2,3 @@
 Host: 127.0.0.1:8990[CR][LF]
 Accept: */*[CR][LF]
 [CR][LF]
-GET /2056 HTTP/1.1[CR][LF]
-Host: 127.0.0.1:8990[CR][LF]
-Authorization: Negotiate IktSQjVfQWxpY2UiOkhUVFBAMTI3LjAuMC4xOjE6QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQQ==[CR][LF]
-Accept: */*[CR][LF]
-[CR][LF]
test 2057...[HTTP Negotiate authentication (stub ntlm)]

 2057: protocol FAILED:
--- log/check-expected  2017-08-24 17:41:24.376379100 +0200
+++ log/check-generated 2017-08-24 17:41:24.372367500 +0200
@@ -2,13 +2,3 @@
 Host: 127.0.0.1:8990[CR][LF]
 Accept: */*[CR][LF]
 [CR][LF]
-GET /2057 HTTP/1.1[CR][LF]
-Host: 127.0.0.1:8990[CR][LF]
-Authorization: Negotiate Ik5UTE1fQWxpY2UiOkhUVFBAMTI3LjAuMC4xOjI6QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQQ==[CR][LF]
-Accept: */*[CR][LF]
-[CR][LF]
-GET /2057 HTTP/1.1[CR][LF]
-Host: 127.0.0.1:8990[CR][LF]
-Authorization: Negotiate Ik5UTE1fQWxpY2UiOkhUVFBAMTI3LjAuMC4xOjM6QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQQ==[CR][LF]
-Accept: */*[CR][LF]
-[CR][LF]

Is this expected?

Will try on openSUSE Leap 42.2 SP2 tonight.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 24, 2017

Works perfectly on the openSUSE Leap 42.2 SP2 from Windows 10.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Aug 26, 2017

Sorry for the delay and thanks for looking at it and for trying it out.

I was able to reproduce the failure on cygwin and I'm looking into it.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Aug 27, 2017

I was able to get around the warning by adding -no-undefined to libstubgss_la_LDFLAGS, however the tests still fail since LD_PRELOAD isn't supported. Other tests using LD_PRELOAD fail as well on cygwin.
I think I'd need to detect if LD_PRELOAD is supported, and disable the test in case it isn't. Thoughts?

iboukris added some commits Jul 18, 2017

tests: add initial gssapi test using stub implementation
The stub implementation is pre-loaded using LD_PRELOAD
and emulates common gssapi uses (only builds if curl is
initially built with gssapi support).

The initial tests are currently disabled for debug builds
as LD_PRELOAD is not used then.
@iboukris

This comment has been minimized.

Contributor

iboukris commented Sep 13, 2017

Hey, I added an exclusion for systems not supporting LD_PRELOAD, as it is currently a requirement.
The gss tests should be skipped now on cygwin. Thanks.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Sep 13, 2017

Great! Sorry for being so unresponsive, I've had no internet access at home for the past 11 weeks. That should change tomorrow, so if all goes well, I'd like to merge this tomorrow if there are no objections.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Sep 14, 2017

Thank you for working on this!

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Sep 15, 2017

tests: add initial gssapi test using stub implementation
The stub implementation is pre-loaded using LD_PRELOAD
and emulates common gssapi uses (only builds if curl is
initially built with gssapi support).

The initial tests are currently disabled for debug builds
as LD_PRELOAD is not used then.

Ref: curl#1687
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment