Re: Make COPY format extendable: Extract COPY TO format implementations

Sutou Kouhei писал(а) 2025-02-04 13:29:
Hi
> Hi,
> 
> In <d838025aceeb19c9ff1db702fa55cabf@postgrespro.ru>
>   "Re: Make COPY format extendable: Extract COPY TO format 
> implementations" on Mon, 03 Feb 2025 13:38:04 +0700,
>   Vladlen Popolitov <v.popolitov@postgrespro.ru> wrote:
> 
>> I would like to inform about the security breach in your design of
>> COPY TO/FROM.
> 
> Thanks! I didn't notice it.
> 
>> You use FORMAT option to add new formats, filling it with routine name
>> in shared library. As result any caller can call any routine in
>> PostgreSQL kernel.
> 
> We require "FORMAT_NAME(internal)" signature:
> 
> ----
> 	funcargtypes[0] = INTERNALOID;
> 	handlerOid = LookupFuncName(list_make1(makeString(format)), 1,
> 								funcargtypes, true);
> ----
> 
> So any caller can call only routines that use the signature.
> 
> Should we add more checks for security? If so, what checks
> are needed?
> 
> For example, does requiring a prefix such as "copy_" (use
> "copy_json" for "json" format) improve security?
> 
> For example, we need to register a handler explicitly
> (CREATE ACCESS METHOD) when we want to use a new access
> method. Should we require an explicit registration for
> custom COPY format too?
> 
> 

I think, in case of USING PostgreSQL kernel will call corresponding 
handler,
and it looks secure - the same as for table and index methods handlers.

>>  Standard PostgreSQL realisation for new methods to use USING
>>  keyword. Every
>> new method could have own options (FORMAT is option of internal 'copy
>> from/to'
>> methods),
> 
> Ah, I didn't think about USING.
> 
> You suggest "COPY ... USING json" not "COPY ... FORMAT json"
> like "CREATE INDEX ... USING custom_index", right? It will
> work. If we use this interface, we should reject "COPY
> ... FORMAT ... USING" (both of FORMAT/USING are specified).
> 
> 
I cannot recommend about rejecting, I do not know details
of realisation of this part of code. Just idea - FORMAT value
could be additional option to copy handler or NULL
if it is omitted.
  If you add extensibility, than every handler will be the
extension, that can handle one or more formats.

>>           it assumes some SetOptions interface, that defines
>> an options structure according to the new method requirements.
> 
> Sorry. I couldn't find the SetOptions interface in source
> code. I found only AT_SetOptions. Did you mean it by "some
> SetOptions interface"?
Yes.
> I'm familiar with only access method. It has
> IndexAmRoutine::amoptions. Is it a SetOptions interface
> example?
Yes. I think, it would be compatible with other modules
of source code and could use the same code base to process
options of COPY TO/FROM
> 
> FYI: The current patch set doesn't have custom options
> support yet. Because we want to start from a minimal feature
> set. But we'll add support for custom options eventually.
Sorry for disturbing. I did not have intention to stop your
patch, I would like to point to that details as early as
possible.


-- 
Best regards,

Vladlen Popolitov.