Re: [PATCH v5 0/5] Debootstrap integration

[Claudius Heine <claudius.heine.ext@siemens.com>]

Hi Baurzhan,

On 04/04/2018 10:34 PM, Baurzhan Ismagulov wrote:
> On Tue, Apr 03, 2018 at 12:07:57PM +0200, claudius.heine.ext@siemens.com wrote:
>> this is the new version of this patchset, that fixes the
>> generate_keyring task in isar-bootstrap for systems with read-only
>> homedir.
> 
> Thanks, worked fine on my host. CI still in progress.
> 
> 
> It's unfortunate that the series introduces regressions you wrote about
> (changing mirrors, setting hostname). It's always better to fix the issues on
> the spot. If there are no objections, I'd like to add TODOs to the patches.
> Please let me know whether it's ok, or you would like to address those before
> the merge.

I don't know about you, but I prefer having TODOs somewhere outside the 
project, maybe in the github issue tracker. At least in my experience 
TODOs together with code or in a separate file inside the repo are 
seldom updated and easily forgotten. If this project prefers having 
TODOs inside the repo, then sure, I have nothing against adding them 
somewhere.

I do plan of adding more features to this once this is merged. This 
patchset just provides the baseline.

> What I'd really like to see is an update to doc/user_manual.md. Would you have
> time for that in the next days?

I'll try.

> 
> 
> If I understand the code correctly, there is also a security issue:

Not sure if security is really a concern for isar yet. But I get your 
point that we should prevent possible accidents. :)

> 
> On Tue, Apr 03, 2018 at 12:08:00PM +0200, claudius.heine.ext@siemens.com wrote:
>> +    CDIRS="${@d.expand(d.getVarFlags("do_build").get("root_cleandirs", ""))}"
>> +    if [ -n "$CDIRS" ]; then
>> +        sudo rm -rf $CDIRS
>> +        mkdir -p $CDIRS
>> +    fi
> 
> Should root_cleandirs items be checked for directory traversal ("/", "..") and
> mounted filesystems in the subdirectories? If yes, do we want to drop the
> feature from this series and address the issue in a separate step?

This isn't really a new feature of isar yet. Its just the start of a 
general interface, that could be use everywhere when its acknowledged by 
the community and fully implemented. So it has to be improved anyway.

So I would say its good enough in this case, since setting those 
directories in the flag and removing them is currently bundled together 
in the same file. If we later centralized this step somewhere 
(base.bbclass) to make it available for every task, then checking it 
more thoroughly has to be done there.
So maybe add centralization of the 'root_cleandirs' task flag to the 
TODO list as well.

Cheers,
Claudius

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de