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

Extend SSL client API #289

Open
wants to merge 1 commit into
base: v6.x.x
Choose a base branch
from

Conversation

andreas-bok-sociomantic
Copy link
Contributor

@andreas-bok-sociomantic andreas-bok-sociomantic commented Apr 19, 2018

Add client API functions to get the cipher name and
protocol version of the established connection

  • Update release notes

Add client API functions to get the cipher name and
protocol version of the established connection
Copy link
Collaborator

@ben-palmer-sociomantic ben-palmer-sociomantic left a comment

Choose a reason for hiding this comment

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

Few comments and a change requested.

@@ -58,6 +58,36 @@ DRIZZLE_API
drizzle_return_t drizzle_set_ssl(drizzle_st *con, const char *key,
const char *cert, const char *ca, const char *capath, const char *cipher);

/**
* @brief Get a pointer to the name of the cipher used current established
* connection.
Copy link
Collaborator

Choose a reason for hiding this comment

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

used in the currently established connection?

* If the cipher is NULL, cipher is set to "(NONE)".
*
* param[in] con A connection object
* param[out] cipher reference to a const char pointer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should Reference be capitalised?

/**
* @brief Get the string which indicates the SSL/TLS protocol version that first
* defined the cipher, or in the case the SSL/TLS protocol negotiated between
* client and server.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to reword the or in the case ... section.

* client and server.
*
* This is currently SSLv2 or TLSv1/SSLv3. In some cases it
* should possibly return "TLSv1.2" but does not;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why doesn't it?

*
* This is currently SSLv2 or TLSv1/SSLv3. In some cases it
* should possibly return "TLSv1.2" but does not;
* If cipher is NULL, "(NONE)" `cipher` is set to "(NONE)".
Copy link
Collaborator

Choose a reason for hiding this comment

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

"(NONE)" cipher is set to "(NONE)"?

}

const SSL_CIPHER *cipher;
cipher = SSL_get_current_cipher((SSL *)con->ssl);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not combine the above two lines?

*cipher_version = SSL_CIPHER_get_version(cipher);
return DRIZZLE_RETURN_OK;
}

#else

drizzle_return_t drizzle_set_ssl(drizzle_st*, const char*, const char*, const char*, const char*, const char*)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this now missing:

{
  return DRIZZLE_RETURN_INVALID_ARGUMENT;
}

@andreas-bok-sociomantic
Copy link
Contributor Author

Thanks for the review. After reading a book on OpenSSL, it seems like the semantics of drizzle_get_cipher is actually different from what I originally though. Since we choose another solution internally for setting up encrypted connections. I will put this on hold for now

@bokchan bokchan modified the milestones: v6.2.0, Future work Dec 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants