Skip to content

tlsio support for http proxy io #512

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

Closed
wants to merge 14 commits into from

Conversation

yunhaoling
Copy link
Contributor

No description provided.

@ericwolz
Copy link
Contributor

ericwolz commented Feb 8, 2021

@yunhaoling
Copy link
Contributor Author

hey @ericwol-msft, I tried to add two tests.

however, I don't know how to give a default value to the bool variable (true/false) in the struct -- I guess it's not supported in C.
I find that during runtime a random true/false could be assigned to the variable, do you know how to resolve the issue in a non-breaking way?

@ericwolz
Copy link
Contributor

yes, this is an issue. You should use http_proxy_io_set_option() to extend the http functionality and not change the io_create_parameters. Move use_tls_proxy to HTTP_PROXY_IO_INSTANCE and add a new option name/value that can be passed in to see this value.

@ewertons

@@ -45,6 +48,7 @@ typedef struct HTTP_PROXY_IO_INSTANCE_TAG
XIO_HANDLE underlying_io;
unsigned char* receive_buffer;
size_t receive_buffer_size;
bool use_tls_io;
Copy link
Contributor

Choose a reason for hiding this comment

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

need to also add the copy code to http_proxy_io_clone_option()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm doing it as if ((strcmp(name, OPTION_UNDERLYING_IO_OPTIONS) == 0) || (strcmp(name, OPTION_USE_TLS_HTTP_PROXY) == 0))

not sure if this is the correct/preferred way

TLSIO_CONFIG tls_io_config;
tls_io_config.hostname = http_proxy_io_instance->proxy_hostname;
tls_io_config.port = http_proxy_io_instance->proxy_port;
http_proxy_io_instance->underlying_io = xio_create(underlying_io_interface, &tls_io_config);
Copy link
Contributor

Choose a reason for hiding this comment

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

is the previous call xio_create in http_proxy_io_create going to fail on proxy with tls mutual auth failure?

Copy link
Contributor Author

@yunhaoling yunhaoling Feb 12, 2021

Choose a reason for hiding this comment

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

http_proxy_io_create won't fail as it's just creating the socket_io instance, no network activity happens at creation time.

it's the socket_dowork that will fail, for example on windows we will get error:

Socketio_Failure: Receiving data from endpoint: 10054.

which is raised from the socketio_win32 do_work code:

int last_error = WSAGetLastError();
if (last_error != WSAEWOULDBLOCK && last_error != ERROR_SUCCESS)
{
    LogError("Socketio_Failure: Receiving data from endpoint: %d.", last_error);
    indicate_error(socket_io_instance);
}


if (http_proxy_io_instance->underlying_io == NULL)
{
http_proxy_io_destroy(http_proxy_io);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be destroying the handle. I don't think we own this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm checking the socket_io creation logic in http_proxy_io_create, I find it would do some check and clean-up work.
do you think we also need to do the check -- tls interface and instance -- and do clean-up work in case of failure?

if (result->underlying_io == NULL)
{
    /* Codes_SRS_HTTP_PROXY_IO_01_012: [ If xio_create fails, http_proxy_io_create shall fail and return NULL. ]*/
    LogError("Unable to create the underlying IO.");
    /* Codes_SRS_HTTP_PROXY_IO_01_008: [ When http_proxy_io_create fails, all allocated resources up to that point shall be freed. ]*/
    free(result->password);
    free(result->username);
    free(result->proxy_hostname);
    free(result->hostname);
    free(result);
    result = NULL;
}


#define PROXY_HOSTNAME "127.0.0.1"
#define PROXY_PORT 8899
#define PROXY_SERVER_CERTIFICATE_DATA "<<<server ssl certificate data which is used by the client to authentcate the identity of the proxy server>>>"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this in PEM format?

{
XIO_HANDLE httpproxyio = (XIO_HANDLE)context;
const char to_send[] = "GET / HTTP/1.1\r\n"
"Host: www.google.com\r\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use google

Copy link

Choose a reason for hiding this comment

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

Would www.contoso.com be ok?

@jessek
Copy link

jessek commented Apr 8, 2021

@yunhaoling: Looks like there is some feedback and suggestions on the code. Can you please update the PR to reflect the requested changes? We are excited to have this capability!

@yunhaoling
Copy link
Contributor Author

hey @jessek , thanks for checking! I was tied up with some python sdk issues last month, but I should have some free cycles this month and will continue the implantation on this one.

@yunhaoling yunhaoling marked this pull request as draft April 14, 2021 19:45
@yunhaoling
Copy link
Contributor Author

closing the PR for now, but we could reactive this in the future if there's new customer wanting it

@yunhaoling yunhaoling closed this Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants