[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".

Re: [FFmpeg-devel] rebasing security

[Alexander Strasser via ffmpeg-devel]

Hi Michael,
hi all!

I think it's a good time to bring stuff like this up for discussion.

On 2025-08-03 21:02 +0200, Michael Niedermayer wrote:
> 
> 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 I understand the original point you wanted to discuss correctly,
than this is not a question of rebase or merge but one of letting
**commits happen on the forge**. If it happens it bears the
possibility of modification on the server the forge is running on.

TL;DR: I think it's fine the way it's setup now.

I'm not against letting rebase/merges happen on the server because
otherwise we would lose a lot of advantages and comfort we get by
using a forge for PRs.

Only alternative I see is to do PRs on the forge and doing merging
manually by the same person that ensures reviewed PR is not changed
and pushes (after rebase or with a clean merge commit) from their
machine.

Just to be clear here I'm talking about stuff happening for
rebase/merge on the server. There is another thing that is
done sometimes that I'm against, that is creating and pushing
commits from runners. That is a way too dangerous practice IMHO
and I would argue we should never do it.


Best regards
  Alexander

PS.
Maybe there are some less conventional possibilities I'm missing, that
could be implemented. So if you see any that seem worth to pursue it
might be interesting.
_______________________________________________
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

[Marton Balint]

On Mon, 4 Aug 2025, Alexander Strasser via ffmpeg-devel wrote:

> Hi Michael,
> hi all!
>
> I think it's a good time to bring stuff like this up for discussion.
>
> On 2025-08-03 21:02 +0200, Michael Niedermayer wrote:
>>
>> 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 I understand the original point you wanted to discuss correctly,
> than this is not a question of rebase or merge but one of letting
> **commits happen on the forge**. If it happens it bears the
> possibility of modification on the server the forge is running on.
>
> TL;DR: I think it's fine the way it's setup now.
>
> I'm not against letting rebase/merges happen on the server because
> otherwise we would lose a lot of advantages and comfort we get by
> using a forge for PRs.
>
> Only alternative I see is to do PRs on the forge and doing merging
> manually by the same person that ensures reviewed PR is not changed
> and pushes (after rebase or with a clean merge commit) from their
> machine.

Two things came to my mind about the current forgejo workflow.

- Previously it was pretty clear from git history who actually committed
   a change from the comitter field. With using forgejo the comitter
   field no longer shows the person who actually *committed* the change to
   the main repo, but it is inherited from the original pull request commit
   instead, so it simply shows the original author of the patch.

- A pull request is writable both by the reviewer and the author up to
   the point when it is actually committed to the main repo. So force
   pushes from an author can happen anytime during this timeline:
     - reviewer reads changes
     - approves the changes
     - rebases the branch
     - sets it up to auto merge
     - CI actually runs
     - forgejo auto-merge
   A reviewer may not realize the new force push from the author. Maybe
   forgejo handle some force pushes in this timeline gracefully and aborts,
   or ignores them, I am not sure. It still looks a bit fragile, my
   expectation as a reviewer would be that what I saw when I finished the
   review and clicked on the Approve button will get comitted, when I later
   click on the merge button.

Regards,
Marton
_______________________________________________
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

[Kacper Michajlow]

On Mon, 4 Aug 2025 at 23:38, Marton Balint <cus@passwd.hu> wrote:
>
>
>
> On Mon, 4 Aug 2025, Alexander Strasser via ffmpeg-devel wrote:
>
> > Hi Michael,
> > hi all!
> >
> > I think it's a good time to bring stuff like this up for discussion.
> >
> > On 2025-08-03 21:02 +0200, Michael Niedermayer wrote:
> >>
> >> 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 I understand the original point you wanted to discuss correctly,
> > than this is not a question of rebase or merge but one of letting
> > **commits happen on the forge**. If it happens it bears the
> > possibility of modification on the server the forge is running on.
> >
> > TL;DR: I think it's fine the way it's setup now.
> >
> > I'm not against letting rebase/merges happen on the server because
> > otherwise we would lose a lot of advantages and comfort we get by
> > using a forge for PRs.
> >
> > Only alternative I see is to do PRs on the forge and doing merging
> > manually by the same person that ensures reviewed PR is not changed
> > and pushes (after rebase or with a clean merge commit) from their
> > machine.
>
> Two things came to my mind about the current forgejo workflow.
>
> - Previously it was pretty clear from git history who actually committed
>    a change from the comitter field. With using forgejo the comitter
>    field no longer shows the person who actually *committed* the change to
>    the main repo, but it is inherited from the original pull request commit
>    instead, so it simply shows the original author of the patch.

I don't think this is accurate. Committer field is set to the person
who clicks the "merge" button. Same as they would manually git push
the patches.

Slightly related, I don't like how simple the web ui commit log of
forgejo is, it doesn't show commiter at all. For me this information
is as important as the author. I'm keeping notes on forgejo usage and
will share it when the time comes, it has some annoying limitations
compared to other forges.

> - A pull request is writable both by the reviewer and the author up to
>    the point when it is actually committed to the main repo. So force
>    pushes from an author can happen anytime during this timeline:
>      - reviewer reads changes
>      - approves the changes
>      - rebases the branch
>      - sets it up to auto merge
>      - CI actually runs
>      - forgejo auto-merge
>    A reviewer may not realize the new force push from the author. Maybe
>    forgejo handle some force pushes in this timeline gracefully and aborts,
>    or ignores them, I am not sure. It still looks a bit fragile, my
>    expectation as a reviewer would be that what I saw when I finished the
>    review and clicked on the Approve button will get comitted, when I later
>    click on the merge button.

There is a timeline of events. If PR is approved, you can see if there
were new events (comments/pushes) after that. This is close to the
"merge" button and you should see "approve" as the last event in the
timeline to be sure nothing changes.

Also if there were rebases after approval, the approved tick mark (in
Reviewers list) becomes yellow instead of green.

It is also possible to configure to completely discard previous
approval if any push happened. But this hinders the workflow if there
is trivial rebase done to run CI on the latest merge base. Reviewer
would have to approve again. This works if the reviewer is the same
person who will merge, but if both author and reviewer are
maintainers, we should trust each other and respect approver to not
include unwanted changes after this point.

Saying that, I think (if possible) it should be configured to clear
the approval status if the **user** (not contributor) pushes to this
branch. This way the reviewer has to recheck it, before merge.

- Kacper
_______________________________________________
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

[Kacper Michajlow]

On Tue, 5 Aug 2025 at 05:06, Kacper Michajlow <kasper93@gmail.com> wrote:
>
> On Mon, 4 Aug 2025 at 23:38, Marton Balint <cus@passwd.hu> wrote:
> >
> >
> >
> > On Mon, 4 Aug 2025, Alexander Strasser via ffmpeg-devel wrote:
> >
> > > Hi Michael,
> > > hi all!
> > >
> > > I think it's a good time to bring stuff like this up for discussion.
> > >
> > > On 2025-08-03 21:02 +0200, Michael Niedermayer wrote:
> > >>
> > >> 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 I understand the original point you wanted to discuss correctly,
> > > than this is not a question of rebase or merge but one of letting
> > > **commits happen on the forge**. If it happens it bears the
> > > possibility of modification on the server the forge is running on.
> > >
> > > TL;DR: I think it's fine the way it's setup now.
> > >
> > > I'm not against letting rebase/merges happen on the server because
> > > otherwise we would lose a lot of advantages and comfort we get by
> > > using a forge for PRs.
> > >
> > > Only alternative I see is to do PRs on the forge and doing merging
> > > manually by the same person that ensures reviewed PR is not changed
> > > and pushes (after rebase or with a clean merge commit) from their
> > > machine.
> >
> > Two things came to my mind about the current forgejo workflow.
> >
> > - Previously it was pretty clear from git history who actually committed
> >    a change from the comitter field. With using forgejo the comitter
> >    field no longer shows the person who actually *committed* the change to
> >    the main repo, but it is inherited from the original pull request commit
> >    instead, so it simply shows the original author of the patch.
>
> I don't think this is accurate. Committer field is set to the person
> who clicks the "merge" button. Same as they would manually git push
> the patches.
>
> Slightly related, I don't like how simple the web ui commit log of
> forgejo is, it doesn't show commiter at all. For me this information
> is as important as the author. I'm keeping notes on forgejo usage and
> will share it when the time comes, it has some annoying limitations
> compared to other forges.
>
> > - A pull request is writable both by the reviewer and the author up to
> >    the point when it is actually committed to the main repo. So force
> >    pushes from an author can happen anytime during this timeline:
> >      - reviewer reads changes
> >      - approves the changes
> >      - rebases the branch
> >      - sets it up to auto merge
> >      - CI actually runs
> >      - forgejo auto-merge
> >    A reviewer may not realize the new force push from the author. Maybe
> >    forgejo handle some force pushes in this timeline gracefully and aborts,
> >    or ignores them, I am not sure. It still looks a bit fragile, my
> >    expectation as a reviewer would be that what I saw when I finished the
> >    review and clicked on the Approve button will get comitted, when I later
> >    click on the merge button.
>
> There is a timeline of events. If PR is approved, you can see if there
> were new events (comments/pushes) after that. This is close to the
> "merge" button and you should see "approve" as the last event in the
> timeline to be sure nothing changes.
>
> Also if there were rebases after approval, the approved tick mark (in
> Reviewers list) becomes yellow instead of green.
>
> It is also possible to configure to completely discard previous
> approval if any push happened. But this hinders the workflow if there
> is trivial rebase done to run CI on the latest merge base. Reviewer
> would have to approve again. This works if the reviewer is the same
> person who will merge, but if both author and reviewer are
> maintainers, we should trust each other and respect approver to not
> include unwanted changes after this point.
>
> Saying that, I think (if possible) it should be configured to clear
> the approval status if the **user** (not contributor) pushes to this
> branch. This way the reviewer has to recheck it, before merge.
>
> - Kacper

Looking quickly at forgejo source, it seems to do a diff against merge
base, so it won't dismiss approvals on trivial rebases. Only if
something actually changes. In this case I think enabling the "dismiss
stale approvals" protection option is a good idea to ensure the
approved state is always valid.

Of course this would clear approve if someone fixes even a typo, but I
guess it is expected.

- Kacper
_______________________________________________
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

[Jacob Lifshay]

On Mon, Aug 4, 2025 at 8:06 PM Kacper Michajlow <kasper93@gmail.com> wrote:
> Slightly related, I don't like how simple the web ui commit log of
> forgejo is, it doesn't show commiter at all. For me this information
> is as important as the author. I'm keeping notes on forgejo usage and
> will share it when the time comes, it has some annoying limitations
> compared to other forges.

It shows both the author and committer if you go to the page for a
particular commit:
https://code.ffmpeg.org/FFmpeg/FFmpeg/commit/157d3b007e93d39483bdc79278911f84b68b3c56

Jacob
_______________________________________________
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".