block .py - Addition(s) to pull clevis and /etc/crypttab data for issue #2755 #2756

cbrashaw · 2021-11-11T23:07:10Z

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

[ x] Is the commit message split over multiple lines and hard-wrapped at 72 characters?
[x ] Is the subject and message clear and concise?
[ x] Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
Does the commit contain a Signed-off-by: First Lastname [email protected]?
Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

Added two commands to block.py LUKS loop to grab "clevis luks list <dev>" and grab a copy of the /etc/crypttab file

…k.py Added block.py via upload to extend the LUKS loop and pull clevis and crypttab data.

packit-as-a-service · 2021-11-11T23:10:12Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/sosreport-sos-2756
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

cbrashaw · 2021-11-12T00:12:28Z

block.py - NBDE/LUKS data - fixed comment to pass flake test.

cbrashaw · 2021-11-12T00:58:31Z

Wanted to try putting the /etc/crypttab pull in the LUKS block - as I'm thinking care should likely be taken when pulling this data (ensure there is a LUKS device present, and not pull any sensitive customer data). If this needs to be moved I can/will move this outside (linearly) with other file/data pulls.

pmoravec · 2021-11-12T07:13:32Z

ACK to the change itself, but:

creating the extra block.py copy is superfluous
the two oldest commits changing the sos/report/plugins/block.py should be squashed into one
one minor change in comment

pmoravec · 2021-11-12T07:17:43Z

sos/report/plugins/block.py

@@ -59,11 +59,13 @@ def setup(self):
        self.add_blockdev_cmd(cmds, blacklist='ram.*')

        lsblk = self.collect_cmd_output("lsblk -f -a -l")
-        # for LUKS devices, collect cryptsetup luksDump
+        # for LUKS devs, pull cryptsetup luksDump, clevis, and /etc/crypttab.


Maybe worth explaining (on an extra comment line) that we need to collect /etc/crypttab when any LUKS device exists, hence add_copy_spec called for every such device. E.g.:

# for LUKS devices, collect cryptsetup, luksDump and clevis # if a LUKS device exists, ensure /etc/crypttab is collected

That will also resolve the concise language due to line length :).

If LUKS device(s) are present ensure we grab a copy of the /etc/crypttab

cbrashaw · 2021-11-12T16:41:31Z

Thank you for the guidance Pavel, implemented your suggestion(s) with regards to pulling /etc/crypttab if LUKS device(s) detected.

…

On Fri, Nov 12, 2021 at 12:18 AM Pavel Moravec ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sos/report/plugins/block.py <#2756 (comment)>: > @@ -59,11 +59,13 @@ def setup(self): self.add_blockdev_cmd(cmds, blacklist='ram.*') lsblk = self.collect_cmd_output("lsblk -f -a -l") - # for LUKS devices, collect cryptsetup luksDump + # for LUKS devs, pull cryptsetup luksDump, clevis, and /etc/crypttab. Maybe worth explaining (on an extra comment line) that we need to collect /etc/crypttab when *any* LUKS device exists, hence add_copy_spec called for every such device. E.g.: # for LUKS devices, collect cryptsetup, luksDump and clevis # if a LUKS device exists, ensure /etc/crypttab is collected That will also resolve the concise language due to line length :). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2756 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AL63NL6PVJ5MDFDGDL6QEADULS5SHANCNFSM5H3RHIPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Kind Regards, Christopher Bradshaw, RHCE Senior Technical Support Engineer ***@***.*** (972) 892-4536 Denver, CO Red Hat <https://www.redhat.com/> <https://red.ht/sig>

cbrashaw · 2021-11-12T18:46:13Z

Hi Pavel, Thanks again for you suggestions. I'm still new to GIT and this project so not sure if I covered all the issues noted; any help in getting this PR squared away would be greatly appreciated :)

pmoravec · 2021-11-13T20:59:16Z

Hello,
I think the final code change is OK. Just squash the 7 commits into one, please :)

One option is to do a rebase, drop&squash individual commits, but this way is more elegant:

git reset --soft HEAD~7   # roll back git history 7 commits (the yours) back, but leave code changes as they are
git commit                # now commit all the changes in one commit
git push --force          # forcefully update your local branch on github

TurboTurtle

Single note below, but otherwise echo'ing @pmoravec on needing the squash the fixups commits into a single commit.

TurboTurtle · 2021-11-17T21:52:38Z

block.py

        if lsblk['status'] == 0:
            for line in lsblk['output'].splitlines():
                if 'crypto_LUKS' in line:
                    dev = line.split()[0]
                    self.add_cmd_output('cryptsetup luksDump /dev/%s' % dev)
+                    self.add_cmd_output('clevis luks list -d /dev/%s' % dev)
+                    self.add_copy_spec('/etc/crypttab')


I'm not following why this needs to be in the for loop?

If there are no LUKS devices, there should be either not be an /etc/crypttab or it should be empty, right? add_copy_spec() gracefully handles when a requested file doesn't exist, and copying an empty/template file doesn't seem all that bad to me.

Is there some condition causing us to not want to collect this file if it exists, yet no LUKS devices are present?

Yes, you are right - there won't be an /etc/crypttab if no LUKS devices are present. I figured inserting it into the loop would be a clean way to keep the block module compact and the LUKS data capture contained in a single loop. We could of course move this outside of the LUKS loop and just grab it anyway - just wanted to minimize the chances of pulling any irrelevant data. I can see why it's redundant as it would pull the file for every LUKS device found; but as you say it does handle situations like that gracefully - so I would defer to your direction here :)

arif-ali · 2024-11-20T15:08:18Z

@cbrashaw is this still something you plan to work on?

cbrashaw · 2024-11-25T23:09:07Z

Hi Arif, Yes, I plan to circle back to this (time permitting). I can always create a new pull request if needed.

…

On Wed, Nov 20, 2024 at 8:08 AM Arif Ali ***@***.***> wrote: @cbrashaw <https://github.com/cbrashaw> is this still something you plan to work on? — Reply to this email directly, view it on GitHub <#2756 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AL63NL2XAX2ZT3AV3DO7EUL2BSQXRAVCNFSM6AAAAABSEZX22GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOBYHAZTGNJTG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Kind Regards, Christopher Bradshaw, RHCE He/Him/His Senior Technical Support Engineer ***@***.*** (972) 892-4536 Denver, CO Red Hat <https://www.redhat.com/> <https://red.ht/sig>

cbrashaw added 3 commits November 11, 2021 15:49

Add files via upload

714f022

Added two commands to block.py LUKS loop to grab "clevis luks list <dev>" and grab a copy of the /etc/crypttab file

Merge pull request #1 from cbrashaw/cbrashaw-NBDE_LUKS_additions.bloc…

845a0d1

…k.py Added block.py via upload to extend the LUKS loop and pull clevis and crypttab data.

Update block.py

02076b1

cbrashaw mentioned this pull request Nov 11, 2021

Additional LUKS/Clevis info for block.py #2755

Open

Update block.py

03adfcb

pmoravec reviewed Nov 12, 2021

View reviewed changes

Updated comments to fix line length.

d13c757

If LUKS device(s) are present ensure we grab a copy of the /etc/crypttab

cbrashaw added 2 commits November 12, 2021 10:25

Delete block.py

c5dac1a

Fixed line length of comment for LUKS loop

55efd64

TurboTurtle requested changes Nov 17, 2021

View reviewed changes

arif-ali added the Status/Need More Info Feedback is required to reproduce issue or to continue work label Nov 20, 2024

jcastill added the Status/RedHat Eng RedHat Engineering has been requested to review label Nov 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

block .py - Addition(s) to pull clevis and /etc/crypttab data for issue #2755 #2756

block .py - Addition(s) to pull clevis and /etc/crypttab data for issue #2755 #2756

Uh oh!

cbrashaw commented Nov 11, 2021 •

edited

Loading

Uh oh!

packit-as-a-service bot commented Nov 11, 2021

Uh oh!

cbrashaw commented Nov 12, 2021

Uh oh!

cbrashaw commented Nov 12, 2021 •

edited

Loading

Uh oh!

pmoravec commented Nov 12, 2021

Uh oh!

pmoravec Nov 12, 2021

Uh oh!

cbrashaw commented Nov 12, 2021 via email

Uh oh!

cbrashaw commented Nov 12, 2021

Uh oh!

pmoravec commented Nov 13, 2021

Uh oh!

TurboTurtle left a comment

Uh oh!

TurboTurtle Nov 17, 2021

Uh oh!

cbrashaw Nov 17, 2021

Uh oh!

arif-ali commented Nov 20, 2024

Uh oh!

cbrashaw commented Nov 25, 2024 via email

Uh oh!

Uh oh!

block .py - Addition(s) to pull clevis and /etc/crypttab data for issue #2755 #2756

Are you sure you want to change the base?

block .py - Addition(s) to pull clevis and /etc/crypttab data for issue #2755 #2756

Uh oh!

Conversation

cbrashaw commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

packit-as-a-service bot commented Nov 11, 2021

Uh oh!

cbrashaw commented Nov 12, 2021

Uh oh!

cbrashaw commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmoravec commented Nov 12, 2021

Uh oh!

pmoravec Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

cbrashaw commented Nov 12, 2021 via email

Uh oh!

cbrashaw commented Nov 12, 2021

Uh oh!

pmoravec commented Nov 13, 2021

Uh oh!

TurboTurtle left a comment

Choose a reason for hiding this comment

Uh oh!

TurboTurtle Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

cbrashaw Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

arif-ali commented Nov 20, 2024

Uh oh!

cbrashaw commented Nov 25, 2024 via email

Uh oh!

Uh oh!

cbrashaw commented Nov 11, 2021 •

edited

Loading

cbrashaw commented Nov 12, 2021 •

edited

Loading