Navigation Menu

Skip to content
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 from

Conversation

iboukris
Copy link
Contributor

@iboukris 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
Copy link
Contributor Author

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.

@iboukris iboukris force-pushed the new_gss_tests branch 3 times, most recently from 7383b5a to 99036cd Compare July 19, 2017 18:46
@MarcelRaad
Copy link
Member

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

@iboukris
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 iboukris force-pushed the new_gss_tests branch 2 times, most recently from d5ffedf to adec429 Compare July 26, 2017 21:20
@iboukris
Copy link
Contributor Author

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
Copy link
Contributor Author

iboukris commented Aug 4, 2017

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

@iboukris
Copy link
Contributor Author

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
Copy link
Member

I'll re-borrow that commit.

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

@MarcelRaad
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@coveralls
Copy link

Coverage Status

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

@iboukris
Copy link
Contributor Author

(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
Copy link

Coverage Status

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

Copy link
Member

@MarcelRaad MarcelRaad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MarcelRaad
Copy link
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
Copy link
Member

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

@iboukris
Copy link
Contributor Author

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
Copy link
Contributor Author

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 iboukris force-pushed the new_gss_tests branch 6 times, most recently from 6f77c27 to 4dfaf4e Compare September 8, 2017 21:52
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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Thank you for working on this!

MarcelRaad pushed a commit to MarcelRaad/curl that referenced this pull request Sep 15, 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 tests are currently disabled for debug builds
as LD_PRELOAD is not used then.

Ref: curl#1687
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants