[FFmpeg-devel] rebasing security

[FFmpeg-devel] rebasing security

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1434 bytes --]

Hi

The "on server rebase" process that we are using with forgejo looks a bit insecure

Previously we wrote code, discussed and then signed and pushed
    In this setup the code coming from a developer is not manipulatable
    because noone else can sign it
    Even if its not signed, stuff would light up if the
    server suddenly changed your pushed commits, as local and
    remote would not match

The current workflow is to create a merge request and up to that we
are good.

The problem, the code is then sometimes rebased on the server, this removes
all signatures and allows arbitrary changes to happen. And that is, after
all reviews.

in the ML based system, a supply chain attack would have to hit author and
all reviewers.
With webapp rebasing a point after the reviews can introduce a change stealthy

The solutions are obvious:
1. ignore security and supply chain attacks
2. use merges not rebases on the server
3. rebase locally, use fast forward only
4. verify on server rebases

whats the oppinon of people about merging instead of rebasing ?
Theres also non security arguments in favor of merges:
    https://lkml.org/lkml/2008/2/12/627

That said, i think "verify on server rebases" is possible, just not
something i have heard off before.

am i missing something ?

thx

--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] rebasing security

[Timo Rothenpieler]

[-- Attachment #1.1: Type: text/plain, Size: 1826 bytes --]

On 8/3/2025 5:31 PM, Michael Niedermayer wrote:
> Hi
> 
> The "on server rebase" process that we are using with forgejo looks a bit insecure
> 
> Previously we wrote code, discussed and then signed and pushed
>      In this setup the code coming from a developer is not manipulatable
>      because noone else can sign it
>      Even if its not signed, stuff would light up if the
>      server suddenly changed your pushed commits, as local and
>      remote would not match
> 
> The current workflow is to create a merge request and up to that we
> are good.
> 
> The problem, the code is then sometimes rebased on the server, this removes
> all signatures and allows arbitrary changes to happen. And that is, after
> all reviews.
> 
> in the ML based system, a supply chain attack would have to hit author and
> all reviewers.
> With webapp rebasing a point after the reviews can introduce a change stealthy
> 
> The solutions are obvious:
> 1. ignore security and supply chain attacks
> 2. use merges not rebases on the server
> 3. rebase locally, use fast forward only
> 4. verify on server rebases
> 
> whats the oppinon of people about merging instead of rebasing ?
> Theres also non security arguments in favor of merges:
>      https://lkml.org/lkml/2008/2/12/627
> 
> That said, i think "verify on server rebases" is possible, just not
> something i have heard off before.
> 
> am i missing something ?
> 
> thx
> 

I can change the setting from "Rebase and merge to FF Only", though that 
would be very tedious to deal with for everyone involved.

Forgejo can keep commit signatures intact if proper keys are configured 
for the users.

I wasn't even aware we used signed commits. Never seen them anywhere 
before. I don't even have a key I could sign commits with.

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4742 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] rebasing security

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 2341 bytes --]

On 8/3/2025 12:38 PM, Timo Rothenpieler wrote:
> On 8/3/2025 5:31 PM, Michael Niedermayer wrote:
>> Hi
>>
>> The "on server rebase" process that we are using with forgejo looks a 
>> bit insecure
>>
>> Previously we wrote code, discussed and then signed and pushed
>>      In this setup the code coming from a developer is not manipulatable
>>      because noone else can sign it
>>      Even if its not signed, stuff would light up if the
>>      server suddenly changed your pushed commits, as local and
>>      remote would not match
>>
>> The current workflow is to create a merge request and up to that we
>> are good.
>>
>> The problem, the code is then sometimes rebased on the server, this 
>> removes
>> all signatures and allows arbitrary changes to happen. And that is, after
>> all reviews.
>>
>> in the ML based system, a supply chain attack would have to hit author 
>> and
>> all reviewers.
>> With webapp rebasing a point after the reviews can introduce a change 
>> stealthy
>>
>> The solutions are obvious:
>> 1. ignore security and supply chain attacks
>> 2. use merges not rebases on the server
>> 3. rebase locally, use fast forward only
>> 4. verify on server rebases
>>
>> whats the oppinon of people about merging instead of rebasing ?
>> Theres also non security arguments in favor of merges:
>>      https://lkml.org/lkml/2008/2/12/627
>>
>> That said, i think "verify on server rebases" is possible, just not
>> something i have heard off before.
>>
>> am i missing something ?
>>
>> thx
>>
> 
> I can change the setting from "Rebase and merge to FF Only", though that 
> would be very tedious to deal with for everyone involved.

Agree it's not ideal. You ask the author to rebase locally, push to 
their branch so it's reflected in the PR, and in the meantime some other 
PR is merged and you need to do it all over again.

> 
> Forgejo can keep commit signatures intact if proper keys are configured 
> for the users.
How does that work? It can't sign the post-rebase commit for you.

> 
> I wasn't even aware we used signed commits. Never seen them anywhere 
> before. I don't even have a key I could sign commits with.
Some people do, the git web interface in g.v.o just doesn't show them. 
Tags are also always signed.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] rebasing security

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2430 bytes --]

Hi

On Sun, Aug 03, 2025 at 05:38:26PM +0200, Timo Rothenpieler wrote:
> On 8/3/2025 5:31 PM, Michael Niedermayer wrote:
> > Hi
> > 
> > The "on server rebase" process that we are using with forgejo looks a bit insecure
> > 
> > Previously we wrote code, discussed and then signed and pushed
> >      In this setup the code coming from a developer is not manipulatable
> >      because noone else can sign it
> >      Even if its not signed, stuff would light up if the
> >      server suddenly changed your pushed commits, as local and
> >      remote would not match
> > 
> > The current workflow is to create a merge request and up to that we
> > are good.
> > 
> > The problem, the code is then sometimes rebased on the server, this removes
> > all signatures and allows arbitrary changes to happen. And that is, after
> > all reviews.
> > 
> > in the ML based system, a supply chain attack would have to hit author and
> > all reviewers.
> > With webapp rebasing a point after the reviews can introduce a change stealthy
> > 
> > The solutions are obvious:
> > 1. ignore security and supply chain attacks
> > 2. use merges not rebases on the server
> > 3. rebase locally, use fast forward only
> > 4. verify on server rebases
> > 
> > whats the oppinon of people about merging instead of rebasing ?
> > Theres also non security arguments in favor of merges:
> >      https://lkml.org/lkml/2008/2/12/627
> > 
> > That said, i think "verify on server rebases" is possible, just not
> > something i have heard off before.
> > 
> > am i missing something ?
> > 
> > thx
> > 
> 
> I can change the setting from "Rebase and merge to FF Only", though that
> would be very tedious to deal with for everyone involved.

that would be "3." in my list and yes it would be tedious, I think the main
question is about the 2./4. options (or if iam missing something)


> 
> Forgejo can keep commit signatures intact if proper keys are configured for
> the users.

Forgejo certainly should have its own key to sign commits it generates.
But it cannot have any individual developers private key.

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] rebasing security

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1685 bytes --]

Hi

On Sun, Aug 03, 2025 at 05:31:39PM +0200, Michael Niedermayer wrote:
[...]
> The solutions are obvious:
> 1. ignore security and supply chain attacks
> 2. use merges not rebases on the server
> 3. rebase locally, use fast forward only
> 4. verify on server rebases

Maybe not everyone understood the problem. So let me try a different
explanation. Without any signatures.

In the ML workflow: (for simplicity we assume reviewer and commiter is the same person)
1. someone posts a patch
2. patch is locally applied or rebased
3. commit is reviewed
4. commit is tested
5. commit is pushed

Here the only way to get bad code in, is through the reviewer
If the reviewer doesnt miss anything and his setup is not compromised
then what he pushes is teh reviewed code

if its manipulated after its pushed git should light up like a christmess tree
on the next "git pull --rebase"


With the rebase on webapp (gitlab or forgejo) workflow
1. someone posts a pull request
2. pr is reviewed
3. pr is approved
4. pr is rebased
5. pr is tested
6, pr is pushed

now here of course the same reviewer trust or compromised scenarios exist
but we also have an extra one and that is the server
because the server strips the signatures during rebase it can modify the
commit. And this happens after review and because a rebase was litterally
requested by the reviewer its not likely to be noticed as something out of
place


thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When you are offended at any man's fault, turn to yourself and study your
own failings. Then you will forget your anger. -- Epictetus

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] rebasing security

[Timo Rothenpieler]

[-- Attachment #1.1: Type: text/plain, Size: 2329 bytes --]

On 8/3/2025 9:02 PM, Michael Niedermayer wrote:
> Hi
> 
> On Sun, Aug 03, 2025 at 05:31:39PM +0200, Michael Niedermayer wrote:
> [...]
>> The solutions are obvious:
>> 1. ignore security and supply chain attacks
>> 2. use merges not rebases on the server
>> 3. rebase locally, use fast forward only
>> 4. verify on server rebases
> 
> Maybe not everyone understood the problem. So let me try a different
> explanation. Without any signatures.
> 
> In the ML workflow: (for simplicity we assume reviewer and commiter is the same person)
> 1. someone posts a patch
> 2. patch is locally applied or rebased
> 3. commit is reviewed
> 4. commit is tested
> 5. commit is pushed
> 
> Here the only way to get bad code in, is through the reviewer
> If the reviewer doesnt miss anything and his setup is not compromised
> then what he pushes is teh reviewed code
> 
> if its manipulated after its pushed git should light up like a christmess tree
> on the next "git pull --rebase"
> 
> 
> With the rebase on webapp (gitlab or forgejo) workflow
> 1. someone posts a pull request
> 2. pr is reviewed
> 3. pr is approved
> 4. pr is rebased
> 5. pr is tested
> 6, pr is pushed
> 
> now here of course the same reviewer trust or compromised scenarios exist
> but we also have an extra one and that is the server
> because the server strips the signatures during rebase it can modify the
> commit. And this happens after review and because a rebase was litterally
> requested by the reviewer its not likely to be noticed as something out of
> place
If you as a pusher of commits want to sign them with your own key, you 
have to do that manually.
There is no sane way for Forgjo to do that for you.

I can configure Forgejo to sign commits it itself generates, that is an 
option. See here for how it can do it on merges.
https://forgejo.org/docs/latest/admin/advanced/signing/#pull-request-merges

I think if I set it to "commitssigned", it'll check all commits in the 
PR against the users configured GPG/SSH key, and if they are all valid, 
it'll then sign them with the instance key whenever it needs to modify 
them for an operation.
"twofa" would also be an option, cause it indicates that the author of 
that commit has some reasonably strong proof that they are them themselves.

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4742 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] rebasing security

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3134 bytes --]

Hi Timo

On Sun, Aug 03, 2025 at 10:01:42PM +0200, Timo Rothenpieler wrote:
> On 8/3/2025 9:02 PM, Michael Niedermayer wrote:
> > Hi
> > 
> > On Sun, Aug 03, 2025 at 05:31:39PM +0200, Michael Niedermayer wrote:
> > [...]
> > > The solutions are obvious:
> > > 1. ignore security and supply chain attacks
> > > 2. use merges not rebases on the server
> > > 3. rebase locally, use fast forward only
> > > 4. verify on server rebases
> > 
> > Maybe not everyone understood the problem. So let me try a different
> > explanation. Without any signatures.
> > 
> > In the ML workflow: (for simplicity we assume reviewer and commiter is the same person)
> > 1. someone posts a patch
> > 2. patch is locally applied or rebased
> > 3. commit is reviewed
> > 4. commit is tested
> > 5. commit is pushed
> > 
> > Here the only way to get bad code in, is through the reviewer
> > If the reviewer doesnt miss anything and his setup is not compromised
> > then what he pushes is teh reviewed code
> > 
> > if its manipulated after its pushed git should light up like a christmess tree
> > on the next "git pull --rebase"
> > 
> > 
> > With the rebase on webapp (gitlab or forgejo) workflow
> > 1. someone posts a pull request
> > 2. pr is reviewed
> > 3. pr is approved
> > 4. pr is rebased
> > 5. pr is tested
> > 6, pr is pushed
> > 
> > now here of course the same reviewer trust or compromised scenarios exist
> > but we also have an extra one and that is the server
> > because the server strips the signatures during rebase it can modify the
> > commit. And this happens after review and because a rebase was litterally
> > requested by the reviewer its not likely to be noticed as something out of
> > place

> If you as a pusher of commits want to sign them with your own key, you have
> to do that manually.
> There is no sane way for Forgjo to do that for you.

yes


>
> I can configure Forgejo to sign commits it itself generates, that is an
> option.

is there a disadvantage ?


> See here for how it can do it on merges.
> https://forgejo.org/docs/latest/admin/advanced/signing/#pull-request-merges

confusing, so many options


> 
> I think if I set it to "commitssigned", it'll check all commits in the PR
> against the users configured GPG/SSH key, and if they are all valid, it'll
> then sign them with the instance key whenever it needs to modify them for an
> operation.
> "twofa" would also be an option, cause it indicates that the author of that
> commit has some reasonably strong proof that they are them themselves.

yeah, I have not thought deeply about it, they seem to want to indicate
something by signing commmits.

To me signing my commits primarly is a way to say the commit was not tampered
with after I signed it.

thx


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
than the original author, trying to rewrite it will not make it better.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] rebasing security

[Timo Rothenpieler]

[-- Attachment #1.1: Type: text/plain, Size: 3052 bytes --]

On 8/3/2025 10:29 PM, Michael Niedermayer wrote:
> Hi Timo
> 
> On Sun, Aug 03, 2025 at 10:01:42PM +0200, Timo Rothenpieler wrote:
>> On 8/3/2025 9:02 PM, Michael Niedermayer wrote:
>>> Hi
>>>
>>> On Sun, Aug 03, 2025 at 05:31:39PM +0200, Michael Niedermayer wrote:
>>> [...]
>>>> The solutions are obvious:
>>>> 1. ignore security and supply chain attacks
>>>> 2. use merges not rebases on the server
>>>> 3. rebase locally, use fast forward only
>>>> 4. verify on server rebases
>>>
>>> Maybe not everyone understood the problem. So let me try a different
>>> explanation. Without any signatures.
>>>
>>> In the ML workflow: (for simplicity we assume reviewer and commiter is the same person)
>>> 1. someone posts a patch
>>> 2. patch is locally applied or rebased
>>> 3. commit is reviewed
>>> 4. commit is tested
>>> 5. commit is pushed
>>>
>>> Here the only way to get bad code in, is through the reviewer
>>> If the reviewer doesnt miss anything and his setup is not compromised
>>> then what he pushes is teh reviewed code
>>>
>>> if its manipulated after its pushed git should light up like a christmess tree
>>> on the next "git pull --rebase"
>>>
>>>
>>> With the rebase on webapp (gitlab or forgejo) workflow
>>> 1. someone posts a pull request
>>> 2. pr is reviewed
>>> 3. pr is approved
>>> 4. pr is rebased
>>> 5. pr is tested
>>> 6, pr is pushed
>>>
>>> now here of course the same reviewer trust or compromised scenarios exist
>>> but we also have an extra one and that is the server
>>> because the server strips the signatures during rebase it can modify the
>>> commit. And this happens after review and because a rebase was litterally
>>> requested by the reviewer its not likely to be noticed as something out of
>>> place
> 
>> If you as a pusher of commits want to sign them with your own key, you have
>> to do that manually.
>> There is no sane way for Forgjo to do that for you.
> 
> yes
> 
> 
>>
>> I can configure Forgejo to sign commits it itself generates, that is an
>> option.
> 
> is there a disadvantage ?
> 
> 
>> See here for how it can do it on merges.
>> https://forgejo.org/docs/latest/admin/advanced/signing/#pull-request-merges
> 
> confusing, so many options
> 
> 
>>
>> I think if I set it to "commitssigned", it'll check all commits in the PR
>> against the users configured GPG/SSH key, and if they are all valid, it'll
>> then sign them with the instance key whenever it needs to modify them for an
>> operation.
>> "twofa" would also be an option, cause it indicates that the author of that
>> commit has some reasonably strong proof that they are them themselves.
> 
> yeah, I have not thought deeply about it, they seem to want to indicate
> something by signing commmits.
> 
> To me signing my commits primarly is a way to say the commit was not tampered
> with after I signed it.

I'll add a key to the instance and enable it in commitssigned mode for 
now. That seems to be the most conservative option for a start.

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4742 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".