Skip to content

Provide getters for min/max proto version#4364

Closed
tiran wants to merge 1 commit intoopenssl:masterfrom
tiran:get_min_max_proto
Closed

Provide getters for min/max proto version#4364
tiran wants to merge 1 commit intoopenssl:masterfrom
tiran:get_min_max_proto

Conversation

@tiran
Copy link
Contributor

@tiran tiran commented Sep 14, 2017

OpenSSL 1.1.0 made SSL_CTX and SSL structs opaque and introduced a new
API to set the minimum and maximum protocol version for SSL_CTX with
TLS_method(). Add getters to introspect the configured versions:

int SSL_CTX_get_min_proto_version(SSL_CTX *ctx);
int SSL_CTX_get_max_proto_version(SSL_CTX *ctx);
int SSL_get_min_proto_version(SSL *ssl);
int SSL_get_max_proto_version(SSL *ssl);

NOTE: The getters do not resolv the version in case when the minimum or
maxium version are configured as '0' (meaning auto-select lowest and
highst version number).

Signed-off-by: Christian Heimes christian@python.org

Checklist
  • documentation is added or updated
  • tests are added or updated

@richsalz richsalz self-assigned this Sep 14, 2017
@richsalz richsalz added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Sep 14, 2017
@tiran
Provide getters for min/max proto version
1201aae 
OpenSSL 1.1.0 made SSL_CTX and SSL structs opaque and introduced a new
API to set the minimum and maximum protocol version for SSL_CTX with
TLS_method(). Add getters to introspect the configured versions:

  int SSL_CTX_get_min_proto_version(SSL_CTX *ctx);
  int SSL_CTX_get_max_proto_version(SSL_CTX *ctx);
  int SSL_get_min_proto_version(SSL *ssl);
  int SSL_get_max_proto_version(SSL *ssl);

NOTE: The getters do not resolv the version in case when the minimum or
maxium version are configured as '0' (meaning auto-select lowest and
highst version number).

Signed-off-by: Christian Heimes <christian@python.org>
@tiran
Copy link
Contributor Author

tiran commented Sep 14, 2017

I addressed the doc-nits issue by adding the macros to util/private.num.

kaduk
kaduk approved these changes Sep 14, 2017
View reviewed changes
Copy link
Contributor

@kaduk kaduk left a comment

Choose a reason for hiding this comment

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

Generally fine, just one question inline

doc/man3/SSL_CTX_set_min_proto_version.pod

These functions return 1 on success and 0 on failure.
These setter functions return 1 on success and 0 on failure. The getter
functions return the configured version or 0 for auto-configuration of
Copy link
Contributor

@kaduk kaduk Sep 14, 2017

Choose a reason for hiding this comment

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

Do we want to give an example and/or pointer to relevant header for what these values would look like (e.g., TLS1_2_VERSION)?

Copy link
Contributor Author

@tiran tiran Sep 15, 2017

Choose a reason for hiding this comment

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

We list the versions a couple lines above. I guess it should be obvious to any reader, don't you think?

Copy link
Contributor

@kaduk kaduk Sep 15, 2017

Choose a reason for hiding this comment

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

Yes, it should be; thanks for pointing it out

@kroeckx
Copy link
Member

kroeckx commented Sep 15, 2017

I guess this could qualify for adding to 1.1.0

@richsalz
Copy link
Contributor

richsalz commented Sep 15, 2017

I agree; we decided missing set/get functions because structures are now opaque is a bugfix.

@richsalz richsalz added the 1.1.0 label Sep 15, 2017
@richsalz richsalz removed their assignment Sep 15, 2017
@richsalz
Copy link
Contributor

richsalz commented Sep 15, 2017

I'm not going to have time to do this, if anyone else wants to merge....

@kaduk
Copy link
Contributor

kaduk commented Sep 15, 2017

I can merge.

levitte pushed a commit that referenced this pull request Sep 15, 2017
@tiran @kaduk
Provide getters for min/max proto version
3edabd3 
OpenSSL 1.1.0 made SSL_CTX and SSL structs opaque and introduced a new
API to set the minimum and maximum protocol version for SSL_CTX with
TLS_method(). Add getters to introspect the configured versions:

  int SSL_CTX_get_min_proto_version(SSL_CTX *ctx);
  int SSL_CTX_get_max_proto_version(SSL_CTX *ctx);
  int SSL_get_min_proto_version(SSL *ssl);
  int SSL_get_max_proto_version(SSL *ssl);

NOTE: The getters do not resolv the version in case when the minimum or
maxium version are configured as '0' (meaning auto-select lowest and
highst version number).

Signed-off-by: Christian Heimes <christian@python.org>

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #4364)
kaduk pushed a commit to kaduk/openssl that referenced this pull request Sep 15, 2017
@tiran @kaduk
Provide getters for min/max proto version
4de3aed 
OpenSSL 1.1.0 made SSL_CTX and SSL structs opaque and introduced a new
API to set the minimum and maximum protocol version for SSL_CTX with
TLS_method(). Add getters to introspect the configured versions:

  int SSL_CTX_get_min_proto_version(SSL_CTX *ctx);
  int SSL_CTX_get_max_proto_version(SSL_CTX *ctx);
  int SSL_get_min_proto_version(SSL *ssl);
  int SSL_get_max_proto_version(SSL *ssl);

NOTE: The getters do not resolv the version in case when the minimum or
maxium version are configured as '0' (meaning auto-select lowest and
highst version number).

Signed-off-by: Christian Heimes <christian@python.org>

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from openssl#4364)

(cherry picked from commit 3edabd3)

Updated for new manual page location and TLS 1.3.
@kaduk kaduk mentioned this pull request Sep 15, 2017
Closed
@kaduk
Copy link
Contributor

kaduk commented Sep 15, 2017

Merged to master; the cherry-pick is not very clean, so that's in #4376 .
Closing this one.

@kaduk kaduk closed this Sep 15, 2017
@tiran tiran deleted the get_min_max_proto branch September 15, 2017 18:17
tiran added a commit to tiran/openssl that referenced this pull request Jan 21, 2018
@tiran
Fix signature of min/max proto getter
ca0adf6 
The getters for min and max proto version wrongly passed NULL instead of
0 as third argument to SSL_ctrl() and SSL_CTX_ctrl(). The third argument
is not used, but the error results in a compiler warning:

warning: passing argument 3 of ‘SSL_CTX_ctrl’ makes integer from pointer without a cast [-Wint-conversion]
int v = SSL_CTX_get_max_proto_version(self->ctx);

See openssl#4364

Signed-off-by: Christian Heimes <christian@python.org>
@tiran tiran mentioned this pull request Jan 21, 2018
Closed
levitte pushed a commit that referenced this pull request Jan 24, 2018
@tiran @kaduk
Fix signature of min/max proto getter
1f82eba 
The getters for min and max proto version wrongly passed NULL instead of
0 as third argument to SSL_ctrl() and SSL_CTX_ctrl(). The third argument
is not used, but the error results in a compiler warning:

warning: passing argument 3 of ‘SSL_CTX_ctrl’ makes integer from pointer without a cast [-Wint-conversion]
int v = SSL_CTX_get_max_proto_version(self->ctx);

See #4364

Signed-off-by: Christian Heimes <christian@python.org>

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #5128)
levitte pushed a commit that referenced this pull request Jan 24, 2018
@tiran @kaduk
Fix signature of min/max proto getter
77882a9 
The getters for min and max proto version wrongly passed NULL instead of
0 as third argument to SSL_ctrl() and SSL_CTX_ctrl(). The third argument
is not used, but the error results in a compiler warning:

warning: passing argument 3 of ‘SSL_CTX_ctrl’ makes integer from pointer without a cast [-Wint-conversion]
int v = SSL_CTX_get_max_proto_version(self->ctx);

See #4364

Signed-off-by: Christian Heimes <christian@python.org>

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #5128)

(cherry picked from commit 1f82eba)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@kaduk kaduk kaduk approved these changes

@richsalz richsalz richsalz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: review pending This pull request needs review by a committer branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tiran @kroeckx @richsalz @kaduk