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

Support for URLs with basic authentication #892

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Sep 7, 2022

This change adds support for basic HTTP authentication values in URLs.

@progtologist
Copy link

This looks exactly as I had it in mind, thanks @cottsay!
However, giving it some further thought, this has one disadvantage (which, honestly, some might not care).
apt is a sudo-only utility. This means that only super users can see the password if the password is stored under /etc/apt/auth.conf.d.
rosdep on the other hand is a user utility. Adding an entry with hardcoded usernames and passwords in /etc/ros/rosdep/sources.list.d makes them visible for all users of that system. On the other hand, creating a similar mechanism to /etc/apt/auth.conf.d also doesn't work, as it would require rosdep to only work with sudo (e.g. rosdep update).

I was thinking of utilizing the gnome-keyring for storing the credentials on a per-user basis, what do you think? If you think it would be a good idea, I can also give it a shot at implementing it!

@cottsay
Copy link
Member Author

cottsay commented Sep 8, 2022

I have mixed feelings about the utility of this change as well, but I also believe it is a simple improvement. Another reason I'm not super happy with this change is that the password is printed to stdout whenever an update is performed. I looked into printing the "sanitized" URL instead, but the best I could do was to repeat the same process in another spot, so it felt forced. Maybe that can be addressed in a follow-up change.

There are some tricks you can utilize to make the passwords less universally readable. You could forego the global rosdep sources list and protect the sources list that contains secrets under your home directory:

$ mkdir -p ~/.ros/rosdep/sources.list.d
$ chmod o-rwx ~/.ros/rosdep/sources.list.d
$ export ROSDEP_SOURCE_PATH=~/.ros/rosdep/sources.list.d
$ rosdep init  # Note: no sudo
$ rosdep update

After that, export the ROSDEP_SOURCE_PATH variable as above in your .bashrc and add the secret URL to a new file in ~/.ros/rosdep/sources.list.d/. You should be able to continue using rosdep and bloom as usual after that.

@cottsay
Copy link
Member Author

cottsay commented Sep 8, 2022

I was thinking of utilizing the gnome-keyring for storing the credentials on a per-user basis, what do you think?

I'm very hesitant to take an unconditional dependency on any gnome libraries just to implement that feature, but I agree that it would be ideal. Maybe there is a lightweight library that can do it opportunistically, but keep in mind that we're still supporting Python 2 in rosdep and many newer libraries don't support that anymore.

@progtologist
Copy link

I will give it a shot in the next days and see what I can come up with, I would also prefer a cleaner and more streamlined solution, but not at the expense of backwards compatibility.
Let me see what I can come up with, worst case scenario, we fallback to your implementation!

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2022

Codecov Report

Base: 74.83% // Head: 74.89% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (1153836) compared to base (b6ad189).
Patch coverage: 91.66% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #892      +/-   ##
==========================================
+ Coverage   74.83%   74.89%   +0.05%     
==========================================
  Files          44       44              
  Lines        3362     3374      +12     
==========================================
+ Hits         2516     2527      +11     
- Misses        846      847       +1     
Impacted Files Coverage Δ
src/rosdep2/url_utils.py 93.75% <91.66%> (-1.25%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Interest in development of support for authentication in rosdep
3 participants