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

SSL_set_verify_depth not exposed by the ssl module #69302

Closed
gbremer mannequin opened this issue Sep 15, 2015 · 10 comments
Closed

SSL_set_verify_depth not exposed by the ssl module #69302

gbremer mannequin opened this issue Sep 15, 2015 · 10 comments
Assignees
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir topic-SSL type-feature A feature request or enhancement

Comments

@gbremer
Copy link
Mannequin

gbremer mannequin commented Sep 15, 2015

BPO 25115
Nosy @vstinner, @tiran
Files
  • verify_depth.patch: Patch for 2.7
  • verify_depth-3.5.patch: Patch for 3.5
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/tiran'
    closed_at = <Date 2018-02-25.20:28:07.355>
    created_at = <Date 2015-09-15.01:31:16.622>
    labels = ['3.7', 'expert-SSL', 'type-feature', 'library']
    title = 'SSL_set_verify_depth not exposed by the ssl module'
    updated_at = <Date 2018-02-25.20:28:07.354>
    user = 'https://bugs.python.org/gbremer'

    bugs.python.org fields:

    activity = <Date 2018-02-25.20:28:07.354>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2018-02-25.20:28:07.355>
    closer = 'christian.heimes'
    components = ['Library (Lib)', 'SSL']
    creation = <Date 2015-09-15.01:31:16.622>
    creator = 'gbremer'
    dependencies = []
    files = ['40471', '40483']
    hgrepos = []
    issue_num = 25115
    keywords = ['patch']
    message_count = 10.0
    messages = ['250718', '250741', '250782', '250842', '301478', '301479', '301838', '301972', '301975', '312854']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'christian.heimes', 'gbremer', 'Alex Gaynor']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25115'
    versions = ['Python 3.7']

    @gbremer
    Copy link
    Mannequin Author

    gbremer mannequin commented Sep 15, 2015

    The SSL_set_verify_depth OpenSSL method is not currently exposed by the ssl module. The context object would seem to be the proper place for it as an instance method.

    @gbremer gbremer mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Sep 15, 2015
    @vstinner
    Copy link
    Member

    + if (depth < 0 || depth > 100) {

    Why 100 and not 10 or 1000?

    SSL_CTX_set_verify_depth() is unable to check the depth?

    The patch lacks unit tests and documentation.

    The patch is for Python 2.7, it would be better to write a patch for the default branch (future Python 3.6).

    @gbremer
    Copy link
    Mannequin Author

    gbremer mannequin commented Sep 15, 2015

    I had thought that I had found documentation that the max depth is 100 and anything higher is ignored -- and as I read that back to me, I believe I read an example passage and interpreted it incorrectly. I'll remove that.

    We primarily use Python 2.7, so I started there. I'll submit another patch with changes on the 3.5 branch and add tests.

    @gbremer
    Copy link
    Mannequin Author

    gbremer mannequin commented Sep 16, 2015

    Attached is a patch for the 3.5 branch. The test is minimal -- we are relying on the underlying OpenSSL library and its context to manage the data. I have removed the data validation from the set function -- OpenSSL seems happy to accept negative numbers for depth, even if that is a non-sensical value. I have started on the documentation, and can do a more comprehensive job if the code section is good or mostly good. I'll do the same for the 2.7 patch.

    @tiran tiran added 3.7 (EOL) end of life topic-SSL labels Sep 8, 2016
    @tiran tiran self-assigned this Sep 15, 2016
    @tiran
    Copy link
    Member

    tiran commented Sep 6, 2017

    The patch looks fairly simple, but what is your use case? I don't like to clobber the SSLContext with additional features. I have never been in a situation that required me to change the verify depths for chain building. Why do you want to restrict or enlarge the verify depths? OpenSSL's default verify depths is sensible.

    @AlexGaynor
    Copy link
    Mannequin

    AlexGaynor mannequin commented Sep 6, 2017

    +1 on making sure we have a concrete use case before expanding the API

    @gbremer
    Copy link
    Mannequin Author

    gbremer mannequin commented Sep 10, 2017

    The use case is for an internal PKI implementation where verification should be, needs to be limited to certificates signed by the PKI CA and no higher to, say, a larger realm which would not be appropriate.

    @tiran
    Copy link
    Member

    tiran commented Sep 12, 2017

    Grant,

    I'm not sure I follow you. Do I understand correctly that you want to call SSL_CTX_set_verify_depth(ctx, 1), in order to enforce that a peer cert is directly signed by your CA?

    That doesn't sound like a good use of SSL_CTX_set_verify_depth(), because it only works for a simple case without an intermediate CA. Most real-world cases have one or more intermediate CAs.

    @AlexGaynor
    Copy link
    Mannequin

    AlexGaynor mannequin commented Sep 12, 2017

    For the use case of "I want to trust this CA, but I don't want to trust any of it's sub CAs" I think there's a simpler solution than expanding our API:

    Create your own cross-sign of the root you want, and add a pathLenConstraint: 0 to the basicConstraints extension.

    By create a cross-sign, I mean a new certificate with the same subject/SPKI/SKI/other-extensions, but instead of being self-signed, sign it under some random private key you throw away. And then use that as your trust root, instead of the original certificate.

    This should work fine for validation.

    @tiran
    Copy link
    Member

    tiran commented Feb 25, 2018

    Both Alex and I agree that verify depth is not the right solution to solve your problem. I'd rather not add more APIs unless they are useful for general audience. OpenSSL has a good default for verify depth.

    @tiran tiran closed this as completed Feb 25, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir topic-SSL type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants