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

Feature/verify ssl flag #536

Closed
wants to merge 4 commits into from

Conversation

itaym120
Copy link

adds verify_ssl flag to indicates if ssl certificate verification is needed (default is True)

Itay hai Meiri and others added 3 commits November 14, 2019 20:11
for those without valid certificates who wish to use twine nonetheless
for those without valid certificates who wish to use twine nonetheless
"-v", "--verify",
default=True,
required=False,
action="store_false",
Copy link
Member

@jaraco jaraco Nov 14, 2019

Choose a reason for hiding this comment

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

This usage seems counter-intuitive to me. If I understand correctly, passing --verify will disable verification. Probably better would be --verify={yes/on/true,no/off/false} and have a default (yes).

default=True,
required=False,
action="store_false",
help="Upload with or without SSL verification."
Copy link
Member

Choose a reason for hiding this comment

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

Please always include a trailing slash.

Suggested change
help="Upload with or without SSL verification."
help="Upload with or without SSL verification.",

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

This pull request needs a changelog entry and also an explanation, either in the pull request or in a linked separate issue, describing the motivation behind this change. Explain why you wanted this feature and why you think others would need it also.

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

I also notice this change has no tests. This change will need tests to ensure it's working as intended and covered by the tests.

@bhrutledge
Copy link
Contributor

This looks similar to PR #463. However, @di expressed some concerns about that in the original issue: #387 (comment).

@bhrutledge bhrutledge requested a review from di November 15, 2019 00:21
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

I'm -1 on this change for the same reasons listed in #387 (comment). Twine shouldn't be providing a crutch to broken/insecure/misconfigured systems.

@itaym120
Copy link
Author

itaym120 commented Nov 15, 2019

I'll close this pr, It's simillar to #463, sorry for that.

@itaym120 itaym120 closed this Nov 15, 2019
James-E-A added a commit to James-E-A/pypa-twine that referenced this pull request Feb 1, 2024
The lack of this has been a *perennial* thorn for people behind
corporate TLS MITM ALG proxies; when it's soluble, it's still
annoying, and sometimes the proxy applications don't use a stable
root bundle, rendering the situation kinda insoluble.

- pypa#328
- pypa#387
- pypa#536
- pypa#740
- pypa#741
- pypa#835
- pypa#915
- pypa#1025
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.

4 participants