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

[options] Add authentication option netrc-cmd #6682

Merged
merged 7 commits into from Jun 21, 2023

Conversation

NDagestad
Copy link
Contributor

@NDagestad NDagestad commented Mar 30, 2023

This adds a new option to specify a command that will be ran by yt-dlp to get the username and password that an extractor needs.
For now I kept the logic of having the passord on the first line of the output, and the username on the second one, I'll change it if you have a specific format in mind you prefer.
I also updated the README, is there another place where documentation should be updated ?

As requested in the issue I opened, I also added the exclusion between the various credential related options.
The exclusion mechanism seems a bit clunky to me, but I can't think of another way that wouldn't be a lot of different comparisons. Maybe there is some python magic I don't know about that makes it possible to do an exclusive comparaison of 3 boolean variables 🤔

Fixes #1706

Template

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • I am the original author of this code and I am willing to release it under Unlicense

What is the purpose of your pull request?

🤖 Generated by Copilot at 11d2237

Summary

🆕📝🛠️

This pull request adds a new feature to yt-dlp that allows users to provide credentials for an extractor using a custom shell command. It introduces a new option --netrc-cmd and updates the relevant code and documentation to support this feature.

If you need to log in to extract
But don't want to store your login facts
You can use --netrc-cmd
To run a shell command
And pass your credentials in tact

Walkthrough

  • Add and document a new option --netrc-cmd to allow the user to provide credentials for an extractor using a custom shell command (link, link, link, link)
  • Validate that the user does not specify more than one of the mutually exclusive options for providing credentials: --username and --password, --netrc-cmd, or --netrc in yt_dlp/__init__.py (link)
  • Pass the value of the --netrc-cmd option to the extractor classes in yt_dlp/__init__.py (link)
  • Import and use the subprocess module to execute the shell command defined by the user in yt_dlp/extractor/common.py (link, link)
  • Update the password hint message to include the --netrc-cmd option in yt_dlp/extractor/common.py (link)
  • Implement a new method _get_netrc_cmd_info to parse the output of the shell command and return the username and password in yt_dlp/extractor/common.py (link)
  • Modify the logic of the _get_login_info method to try the --netrc-cmd option before the --netrc option if the user did not provide the username and password manually in yt_dlp/extractor/common.py (link)

@pukkandan
Copy link
Member

For ref: some of the disucussion is in #6676

@pukkandan pukkandan added the enhancement New feature or request label Mar 31, 2023
@pukkandan
Copy link
Member

pukkandan commented Mar 31, 2023

I also updated the README, is there another place where documentation should be updated ?

usenetrc: Use netrc for authentication instead.

The exclusion mechanism seems a bit clunky to me, but I can't think of another way that wouldn't be a lot of different comparisons. Maybe there is some python magic I don't know about that makes it possible to do an exclusive comparaison of 3 boolean variables 🤔

if sum(map(bool, (opts.usenetrc, opts.netrc_cmd, opts.username))) > 1:

For now I kept the logic of having the passord on the first line of the output, and the username on the second one, I'll change it if you have a specific format in mind you prefer.

See #6676 (comment), but I am open to discussion

@NDagestad
Copy link
Contributor Author

From #6676 :

I was thinking we expect netrc format? So --netrc-cmd "cat ~/.netrc" will work and other commands can return multiple machines if needed without parsing extractor key.

I though about doing that in the beginning, but it was more work and it mean every command would need to format their output like the netrc command. I can see the usefulness of being able to have the command be a nearly drop in replacement for the encrypted .netrc file. But the netrc library takes a path as argument, and that is surprisingly annoying to work around.
I have a few solutions that can work, but they all have their drawbacks:

  • Use a plain file instead of the FIFO. This has the disadvantage of writing the credentials to the disk, which I would imagine most people who don't want their password in a clear text file won't be very happy about.
  • Create a FIFO, redirect the output to it and pass it to the netrc lib. Nothing is written on disk but for this, we need to use the low level os.{open,read,write} functions, which I think are not portable (it looks like we want to support Windows as well and not just unixes).
  • Parse the netrc format by hand <- not a big fan of this idea even if it looks like a very simple format
  • Patch the netrc library to be able to take a file object instead of a path. This would be the best of these options, but it might take a very long time to trickle down in the different distributions if it is accepted upstream at all.

In the end, I think it is better to have a very simple format for the command output, and rather have people do the parsing in the command itself, something like gpg -d ~/.authinfo.gpg | sed -n 's/machine {} login \(.*\) password \(.*\)/\2\n\1/p' could be enough for that kind of usecase.
But the format "password\nusername" is not something I strongly attached to,

%(...)s support can also be added in future if needed like we did for --exec. {} is fine for now. But should it be replaced with ie_key() or with _NETRC_MACHINE? I think the latter is better.

the _get_netrc_login_info handler function uses _NETRC_MACHINE, I think it makes sens for them to both uses the same selector, the --netrc-cmd "cat ~/.netrc" behaviour would not work for all extractors if they uses different ones. (e.g. Nebula at least has different values for it)

@pukkandan
Copy link
Member

But the netrc library takes a path as argument, and that is surprisingly annoying to work around.

Untested, but something like this should work:

class stream_netrc(netrc.netrc):
    def __init__(self, stream):
        self.hosts = {}
        self.macros = {}
        self._parse('-', stream, False)

Ref: https://github.com/python/cpython/blob/3.11/Lib/netrc.py#L67-L78

I think it makes sense for them to both uses the same selector,

Agreed

@NDagestad
Copy link
Contributor Author

Untested, but something like this should work:

...

Ref: https://github.com/python/cpython/blob/3.11/Lib/netrc.py#L67-L78

Oh well, that worked perfectly!
Do you want that class somewhere accessible so it can be reused by other parts of the yt-dlp ?

@pukkandan
Copy link
Member

put it in utils.py

@NDagestad
Copy link
Contributor Author

Sorry I didn't notify that I applied the changes suggested, so I don't know I you have seen it.
No rush though.

@NDagestad NDagestad force-pushed the master branch 4 times, most recently from c2d3bb8 to af76cdc Compare April 15, 2023 11:59
@NDagestad
Copy link
Contributor Author

Rebased on latest commits; is there something blocking for this to get merged ?

@bashonly
Copy link
Member

bashonly commented May 6, 2023

In the future, please avoid force-pushing in the middle of review. You can just do a merge commit

It will be reviewed when time permits

@pukkandan pukkandan force-pushed the master branch 2 times, most recently from ee280c7 to 7aeda6c Compare May 24, 2023 18:09
README.md Outdated Show resolved Hide resolved
@pukkandan
Copy link
Member

48525a6 is not fully tested coz Ive gtg. Once it is, and the doc is corrected, this can be merged.

@pukkandan pukkandan added the needs-testing Patch needs testing label May 29, 2023
@pukkandan pukkandan self-assigned this May 29, 2023
Authored by: NDagestad
Authored by: NDagestad
@NDagestad
Copy link
Contributor Author

From my testing this works without problem. Maybe the example is not the most pertinent though, maybe replacing it with --netrc-cmd "gpg --decrypt ~/.authinfo.gpg 2> /dev/null" like proposed in #1706 would make sens to more people.

@pukkandan
Copy link
Member

pukkandan commented May 29, 2023

Maybe the example is not the most pertinent though, maybe replacing it with --netrc-cmd "gpg --decrypt ~/.authinfo.gpg 2> /dev/null" like proposed in #1706 would make sens to more people.

I agree that is easier to understand.

PS: 2>devnul isnt even needed making it even simpler.

@pukkandan pukkandan removed the needs-testing Patch needs testing label May 29, 2023
@pukkandan pukkandan merged commit db3ad8a into yt-dlp:master Jun 21, 2023
11 checks passed
aalsuwaidi pushed a commit to aalsuwaidi/yt-dlp that referenced this pull request Apr 21, 2024
Authored by: NDagestad, pukkandan
Closes yt-dlp#1706
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use authinfo.gpg instead of netrc for authentication
3 participants