-
Notifications
You must be signed in to change notification settings - Fork 5
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
EBLINUX-27055: adding credential support #38
Conversation
06b2af0
to
dfb74a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be highly beneficial for the user experience if there is not only one netrc-like file with authentication information, but a directory where all files in that are used. This allows simpler management in case multiple credentials are required (and I guess it simplifies automation)
Change requests
- Take all files in /workspace/tools/user_config/auth.d/*.conf. For the .netrc file, they of course have to be merged into one file. I think for apt it is possible to use multiple files.
- If the path does not exist: Just ignore it. That fixes the tests here
- Add tests to ensure that this works and is not broken. The most important thing to test is that no credentials are left in the image. I am not sure how to implement a generic test, that ensures that credentials work, though. The only thing I can think of right now would be to spawn an http server in the test, that can then be used for verification.
c496995
to
8cbee2f
Compare
ebcl/common/config.py
Outdated
def _create_netrc_file(self) -> None: | ||
""" combine credential files and create .netrc """ | ||
if not os.path.isdir(self.cred_dir): | ||
return | ||
|
||
dest = os.path.join(self.cred_dir, '../.netrc') | ||
# Ensure .netrc file is empty when starting | ||
with open(dest, 'w') as outfile: | ||
outfile.write('') | ||
|
||
with open(dest, 'a') as outfile: | ||
for f in os.listdir(self.cred_dir): | ||
if f.endswith('.conf'): | ||
with open(os.path.join(self.cred_dir, f), 'r') as infile: | ||
shutil.copyfileobj(infile, outfile) | ||
outfile.write('\n') | ||
|
||
if self.use_fakeroot: | ||
self.fake.run_cmd(f'install -m 600 {dest} $HOME/.netrc') | ||
else: | ||
self.fake.run_sudo(f'install -m 600 {dest} $HOME/.netrc') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is installing the .netrc file for the current user in the container (to make python's requests module use the .netrc) right?
In that case there is no need to use self.fake
Just open the ".netrc" file and write to it:
with (Path.home() / ".netrc").open("w") as outfile:
# write content
There is also no need to open the file with "w" first and then with "a". Just open with "w" and write to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted and same is updated
ebcl/common/config.py
Outdated
self.fake.run_cmd(f'rm -rf {self.target_dir}; [[ -f $HOME/.netrc ]] && rm $HOME/.netrc || true') | ||
else: | ||
self.fake.run_sudo(f'rm -rf {self.target_dir}') | ||
self.fake.run_sudo(f'rm -rf {self.target_dir}; [[ -f $HOME/.netrc ]] && rm $HOME/.netrc || true') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real need to remove the file here. But it could be done for security.
But just use (Path.home() / ".netrc").unlink(missing_ok=True)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted and same is updated by using python code instead of shell
ebcl/tools/root/debootstrap.py
Outdated
|
||
def _remove_credentials(self): | ||
""" cleanup user credentials """ | ||
if not [f for f in os.listdir(self.cred_dir) if f.endswith('.conf')]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either not os.path.isdir(self.cred_dir)
is missing here or it is not needed in _copy_credentials
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted and same is updated
supporting credentials to be used part of the target image creation Jira-ID: EBLINUX-27055
adding support to find or get metadata about a apt repo which requires credentials Jira-ID: EBLINUX-27055
8cbee2f
to
40d84c2
Compare
Superseded by #52 |
Adding support to image creation operations to use the user provided credentials.
config.py: adding credition support to container
- adding support to find or get metadata about a apt repo which requires credentials
debootstrap: supporting user credentials
- supporting credentials to be used part of the target image creation
Jira-ID: EBLINUX-27055
Dependencies:
Elektrobit/ebcl_template#90