generate: do not validate caps when being dropped

[runcom]

There's actually little value in erroring out when asked to drop a
capability not supported on a system. This patch simply removes the cap
validation and just go ahead and drop the capability.

@mrunalp @Mashimiao PTAL

Signed-off-by: Antonio Murdaca runcom@redhat.com

[mrunalp]

LGTM

[mrunalp]

@vbatts PTAL

[Mashimiao]

Sorry, I don't think that's validation's problem. From the log in reference PR #860, it seems you have given wrong cp value CAP_CAP_AUDIT_READ. I think the right value should be CAP_AUDIT_READ

[wking]

... it seems you have given wrong cp value CAP_CAP_AUDIT_READ. I think the right value should be CAP_AUDIT_READ

How about checking for "valid or already in the array (even if not valid)" and erroring if neither are true?

[Mashimiao]

If you use AddProcessCapability() to add cap, you even can't put wrong value into the cap array.
But maybe someone added wrong value into config.json directly, we can't control such thing.
So, I think your suggestion is OK.
We can move CapValid to behind the cap removing loop, if the value is not in array and also not a valid value, we can return the error

[runcom]

@Mashimiao no, this is not about the log in the linked PR, the CAP_CAP_ was a runtime-tools bug ;) fixed by my last PR here.

Anyhow, it's OK to validate to add caps, but totally useless to validate to DROP. What's the point in validating on drop? What do you gain? We aren't validating what will be put in the config like this, dropping removes a value so it's totally fine to not validate.

[Mashimiao]

On 09/08/2017 04:52 PM, Antonio Murdaca wrote: @Mashimiao <https://github.com/mashimiao> no, this is not about the log in the linked PR, the CAP_CAP_ was a runtime-tools bug ;) fixed by my last PR here.

Oh, sorry for my mistake.

Anyhow, it's OK to validate to add caps, but totally useless to validate to DROP. What's the point in validating on drop? What do you gain? We aren't validating what will be put in the config like this, dropping removes a value so it's totally fine to not validate

I want to keep cap validation for drop because I'm afraid users may accidentally input wrong value, we can return him an error to warn that  the value is not right. But letting users to remove an exist value from array is reasonable, So, I suggested to move cap validation behind the removal loop in DropProcessCapabilities().

[runcom]

I want to keep cap validation for drop because I'm afraid users may accidentally input wrong value

@Mashimiao sorry but I can't understand this. What's the point in validating something that will be removed? If I want to remove a cap that my OS doesn't support, what's the issue? Who cares? If my OS doesn't support it, it's already dropped and will be discarded on capability application. If that's not the case, probably you're right though.

[Mashimiao]

I think you didn't get my point. I said,

 I'm afraid users may accidentally input wrong value, we can return him an error to warn that

For example, If user want drop CAP_SYS_ADMIN, accidentally type CAP_SYS_ADMI, I think we should tell him he's trying to drop invalid value. And that's what I care about.

[runcom]

@Mashimiao in that case, I believe we just need to turn off host validation, that way, you only care about syntax. PTAL, I pushed now.

[Mashimiao]

LGTM

[wking]

Turning off host-specific checks won't cover the "someone got a broken value in there, and I want to remove it" case @Mashimiao pointed out here.

[runcom]

Turning off host-specific checks won't cover the "someone got a broken value in there, and I want to remove it" case @Mashimiao pointed out here.

And I believe you should check code https://github.com/opencontainers/runtime-tools/blob/master/validate/validate.go#L781 which disables check on host but keeps "valid cap" check...

That is just covering supportability on host, we don't need that, and that was the original error reported by me which I was trying to fix.

This is good now.

[wking]

And I believe you should check code https://github.com/opencontainers/runtime-tools/blob/master/validate/validate.go#L781 which disables check on host but keeps "valid cap" check...

There is a CapValid check thst is independent of hostSpecific [here][1], which would block you from removing I_DONT_START_WITH_CAP_ if that happened to already be in the array.

[Mashimiao]

@runcom thinks this is enough for him, I'm OK with this. I plan to make a follow PR to fix that issue.

[mrunalp]

LGTM