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

sspi: print out InitializeSecurityContext() error message #1395

Closed
wants to merge 1 commit into from

Conversation

iboukris
Copy link
Contributor

@iboukris iboukris commented Apr 6, 2017

Signed-off-by: Isaac Boukris iboukris@gmail.com
Assisted-by: Jay Satiro
Reported-by: Carsten (talksinmath)

See #1384

@mention-bot
Copy link

@Frenche, thanks for your PR! By analyzing the history of the files in this pull request, we identified @captain-caveman2k, @bagder and @mirekfranc to be potential reviewers.

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.

Why does this return CURLE_OUT_OF_MEMORY? Most of the other possible return values of InizializeSecurityContext seem more likely to me...

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

I don't think See is a parseable keyword, when you are referencing a bug please use Bug: or if it's a github issue you could use one of the github auto keywords instead like Fixes. hre are some examples, check the repo history for more

-- blank line --
Bug: https://github.com/curl/curl/issues/1384
Reported-by: Carsten (talksinmath)

Closes #1395
Reported-by: Carsten (talksinmath)

Fixes https://github.com/curl/curl/issues/1384
Closes https://github.com/curl/curl/pull/1395
Reported-by: Carsten (talksinmath)

Fixes #1384
Closes #1395

@jay
Copy link
Member

jay commented Apr 6, 2017

Why does this return CURLE_OUT_OF_MEMORY?

It looks like it returns that for all errors that contain 0x80000000 or in other words all SEC_E_ errors. I can't think of a good catch all there, maybe CURLE_RECV_ERROR

@iboukris
Copy link
Contributor Author

iboukris commented Apr 6, 2017

I think OOM is used as a generic error.
btw, are we ok with dereferencing data->easy_conn ? I think it should be ok.

@@ -224,6 +225,8 @@ CURLcode Curl_auth_decode_spnego_message(struct Curl_easy *data,
free(chlg);

if(GSS_ERROR(nego->status)) {
failf(data, "InitializeSecurityContext failed: %s",
Curl_sspi_strerror(data->easy_conn, nego->status));
return CURLE_OUT_OF_MEMORY;
Copy link
Member

@bagder bagder Apr 6, 2017

Choose a reason for hiding this comment

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

I'm with @jay, CURLE_RECV_ERROR might be slightly better. But none of them is ideal here...

@iboukris
Copy link
Contributor Author

iboukris commented Apr 6, 2017

btw, are we ok with dereferencing data->easy_conn ?

I think it's ok because Curl_auth_decode_spnego_message() is called from Curl_input_negotiate() which is called with 'conndata' (assuming conn->data->easy_conn is circular).

@jay
Copy link
Member

jay commented Apr 6, 2017

yes it's fine

Reported-by: Carsten (talksinmath)

Fixes curl#1384
Closes curl#1395
@iboukris
Copy link
Contributor Author

iboukris commented Apr 6, 2017

I've updated the commit message. Thanks all for the reviews.

@jay
Copy link
Member

jay commented Apr 6, 2017

LGTM

@MarcelRaad MarcelRaad closed this in 1f152a4 Apr 7, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 23, 2018
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.

5 participants